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

P argument deprecation #5293

Merged
merged 8 commits into from Jun 19, 2019

Conversation

Projects
None yet
5 participants
@DodoMomo
Copy link
Contributor

commented Jun 4, 2019

Changelog: Fix: Deprecated conan copy|download|upload <ref> -p=ID, use conan .... <pref> instead
Docs: conan-io/docs#1317

Fixes #5250
Closes #5225

  • Refer to the issue that supports this Pull Request.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.
@CLAassistant

This comment has been minimized.

Copy link

commented Jun 4, 2019

CLA assistant check
All committers have signed the CLA.

@DodoMomo DodoMomo force-pushed the DodoMomo:p_argument_deprecation branch from b86bc5e to 9acacf5 Jun 5, 2019

@DodoMomo DodoMomo marked this pull request as ready for review Jun 5, 2019

@danimtb

danimtb approved these changes Jun 6, 2019

Copy link
Member

left a comment

Thanks a lot for the contribution!!

Looks good from my side. Just take a look at the suggestion for the parameter name in the api

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

@danimtb danimtb self-assigned this Jun 6, 2019

@memsharded
Copy link
Contributor

left a comment

This PR is good, with good tests, and everything clear. Great job, thanks very much.

Assigning it to next release 1.17

# Copy just one
client.run("copy Hello0/0.1@lasote/stable pepe/stable -p %s" % packages[0])
# Copy just one with --package argument
client.run("copy Hello0/0.1@lasote/stable:%s pepe/stable" % packages[0])

This comment has been minimized.

Copy link
@memsharded

memsharded Jun 6, 2019

Contributor

I think I would leave one test (for each command) that keeps the old -p=ID argument, to make sure that we are not breaking existing users.

This comment has been minimized.

Copy link
@DodoMomo

DodoMomo Jun 7, 2019

Author Contributor

OK, I will take that as an opportunity to refactor the tests a bit.

@memsharded memsharded added this to the 1.17 milestone Jun 6, 2019

@DodoMomo DodoMomo force-pushed the DodoMomo:p_argument_deprecation branch 2 times, most recently from 323ba2e to 58804f6 Jun 10, 2019

@DodoMomo DodoMomo force-pushed the DodoMomo:p_argument_deprecation branch from 58804f6 to 9273a80 Jun 11, 2019

@DodoMomo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

I'm sorry i messed up with the last commit.

@jgsogo
Copy link
Member

left a comment

Thank you very much! 👍

I'm just suggesting to change some typos I've found

Show resolved Hide resolved conans/client/command.py Outdated
Show resolved Hide resolved conans/client/command.py Outdated
Show resolved Hide resolved conans/client/command.py Outdated
Show resolved Hide resolved conans/client/conan_api.py Outdated
Fix typos
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
@jgsogo

jgsogo approved these changes Jun 19, 2019

Copy link
Member

left a comment

Thanks!

@danimtb danimtb merged commit df8fd41 into conan-io:develop Jun 19, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
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.