Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cli] rename copy-files to transfer (Fixes #748) #778

Merged
merged 3 commits into from
May 10, 2019
Merged

Conversation

Saviq
Copy link
Collaborator

@Saviq Saviq commented May 10, 2019

No description provided.

@Saviq Saviq force-pushed the rename-copy-transfer branch 2 times, most recently from c9cd6ed to b1cdf96 Compare May 10, 2019 09:19
@ricab ricab self-requested a review May 10, 2019 11:23
Copy link
Collaborator

@ricab ricab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good. Maintaining the alias and description sounds reasonable. Only a question and a couple of things missing:

  • error msg in transfer.cpp:73
  • bash completion

return QStringLiteral("Transfer files between the host and instances");
}

QString cmd::Transfer::description() const
{
// TODO: Don't mention directories until we support that
// return QStringLiteral("Copy files and directories between the host and instances.");
return QStringLiteral("Copy files between the host and instances.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good choice leaving this as is (clarifies what is meant by transfer).

{
return QStringLiteral("Copy files between the host and instances");
return {name(), "copy-files"};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice this is not reflected in the help, so the alias is hidden. Is that intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we've just left it here for muscle-memory purposes.

@Saviq Saviq force-pushed the rename-copy-transfer branch from b1cdf96 to f47b38c Compare May 10, 2019 12:37
@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #778 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #778      +/-   ##
=========================================
+ Coverage   67.59%   67.6%   +0.01%     
=========================================
  Files         176     176              
  Lines        6171    6173       +2     
=========================================
+ Hits         4171    4173       +2     
  Misses       2000    2000
Impacted Files Coverage Δ
src/platform/client/client_platform.cpp 92.85% <100%> (ø) ⬆️
src/client/cmd/transfer.cpp 74.46% <100%> (ø)
src/client/client.cpp 87.5% <100%> (ø) ⬆️
src/client/cmd/transfer.h 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc2d5d6...f47b38c. Read the comment docs.

@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #778 into master will increase coverage by 0.01%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #778      +/-   ##
=========================================
+ Coverage   67.59%   67.6%   +0.01%     
=========================================
  Files         176     176              
  Lines        6171    6173       +2     
=========================================
+ Hits         4171    4173       +2     
  Misses       2000    2000
Impacted Files Coverage Δ
src/platform/client/client_platform.cpp 92.85% <100%> (ø) ⬆️
src/client/client.cpp 87.5% <100%> (ø) ⬆️
src/client/cmd/transfer.h 100% <100%> (ø)
src/client/cmd/transfer.cpp 74.46% <92.3%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc2d5d6...565a4c3. Read the comment docs.

Copy link
Collaborator Author

@Saviq Saviq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* error msg in transfer.cpp:73
* bash completion

Fixed. Always the bash completion ;)

{
return QStringLiteral("Copy files between the host and instances");
return {name(), "copy-files"};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we've just left it here for muscle-memory purposes.

@Saviq Saviq force-pushed the rename-copy-transfer branch from f47b38c to a0c9bb8 Compare May 10, 2019 12:39
@ricab ricab self-requested a review May 10, 2019 14:08
Copy link
Collaborator

@ricab ricab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

bors bot added a commit that referenced this pull request May 10, 2019
778: [cli] rename `copy-files` to `transfer` (Fixes #748) r=ricab a=Saviq



Co-authored-by: Michał Sawicz <[email protected]>
Co-authored-by: Ricardo Abreu <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 10, 2019

Build failed

Copy link
Collaborator

@ricab ricab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

bors bot added a commit that referenced this pull request May 10, 2019
778: [cli] rename `copy-files` to `transfer` (Fixes #748) r=ricab a=Saviq



Co-authored-by: Michał Sawicz <[email protected]>
Co-authored-by: Ricardo Abreu <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 10, 2019

@bors bors bot merged commit 565a4c3 into master May 10, 2019
@bors bors bot deleted the rename-copy-transfer branch May 10, 2019 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants