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] Accept a PackageReference in the command 'get' (preferred) #4494

Merged
merged 13 commits into from Feb 18, 2019

Conversation

Projects
None yet
5 participants
@jgsogo
Copy link
Member

commented Feb 8, 2019

Changelog: Feature: Accept a PackageReference for the command conan get (argument -p is accepted, but hidden)
Docs: conan-io/docs#1070

closes #4482 <- conan get <package_reference> ...
closes #4479 <- UX: remove colon when files are not shown in the Copied .... message

I think that the call with the full reference should be preferred over the one with the -p argument, so I'm hiding that parameter but still accepting it. Nevertheless, we have to approve explicitly which one to favor.

I've also moved the get_path functionality inside the package layouts.

@ghost ghost assigned jgsogo Feb 8, 2019

@ghost ghost added the stage: review label Feb 8, 2019

@jgsogo jgsogo changed the title Accept a PackageReference in the command 'get' CLI (preferred option) [CLI] Accept a PackageReference in the command 'get' (preferred) Feb 8, 2019

@jgsogo jgsogo requested a review from memsharded Feb 8, 2019

@memsharded
Copy link
Contributor

left a comment

Very good. Just a small detail, please remove the "finally" and can be merged.

Show resolved Hide resolved conans/client/command.py Outdated

@memsharded memsharded added this to the 1.13 milestone Feb 9, 2019

@danimtb
Copy link
Member

left a comment

👍 Would like to see a test with both package_ref and package_id, like $ conan get pkg/1.0@user/channel:120329 -p 23r2432

@danimtb

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Forgot to mention that it would be nice to have somewhere in the docs the definitions for "reference" and "package reference" and "package id" (also for "recipe" and the controversial "package", but that is another story)

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

Much better now, please @danimtb review it again (I've added the test you were requesting) and consider merging. Thanks!

@danimtb

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

LGTM. Add the docs and I will merge accordingly 😀

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

Done! Thanks for the kindly reminder...

if package_id:
self._user_io.out.warn("Usage of `--package` argument is deprecated."
" Use a full reference instead: "
"`conan get [...] {}:{}`".format(reference, package_id))

This comment has been minimized.

Copy link
@uilianries

uilianries Feb 11, 2019

Member

I liked it, dont need to think, just copy and go.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Feb 11, 2019

Author Member

For the copy/paste I need to unroll the other arguments... maybe I should do it 🤔

This comment has been minimized.

Copy link
@uilianries

uilianries Feb 11, 2019

Member

well, it would be really great for lazy people (including me) 😹

This comment has been minimized.

Copy link
@SSE4

SSE4 Feb 11, 2019

Contributor

yes, would be nice to keep it

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

I think that everything is ready, isn't it? 💫

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

I am fine with get_content.

@danimtb

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

@jgsogo have a look at the review comments and solve conflicts

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

Not a strong opinion, but there are several items that should have the same nomenclature for consistency:

  • the argument in the CLI is path (we can change it because it is a positional one)
  • the function in the conan_api is get_path (we can change it...)
  • the function in the layout, change suggested from get_path to get_content.
  • functions in the remotes/api related to this that are called get_path.

So, although I prefer get_content too, I'm not sure if it is better to keep the get_path nomenclature. I'm going to fix the conflicts, I'll read your opinion when I'm back.

@uilianries

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Let's use get_path and get_content just to confuse all programmers 😈

Now really, if we have the opportunity to change all those methods and follow the correct representation of them, so I think it will be better. When I read get_path I understand that function will return a file/dir path only.

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

@uilianries, @memsharded, I'm not going to do the renaming in this PR, nevertheless I've written down it here (#4132 (comment)): still we need to find a better name than get_content, so feel free to comment there so we can address it in a future PR.

Thanks.

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

@danimtb, waiting for your merge 🎶

@danimtb danimtb merged commit 8f72faa into conan-io:develop Feb 18, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Feb 18, 2019

@jgsogo jgsogo deleted the jgsogo:fix/command-get branch Feb 18, 2019

@lasote lasote referenced this pull request Feb 18, 2019

Open

get_path refactor #4549

@memsharded memsharded referenced this pull request Feb 18, 2019

Merged

fix develop #4560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.