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

Handling --upgrade-strategy parameter for editable installs #6648

Closed
3lnc opened this issue Jun 25, 2019 · 13 comments

Comments

@3lnc
Copy link

commented Jun 25, 2019

Environment

  • pip version: 19.1..1
  • Python version: 3.7.1
  • OS: linux 4.15

Description
Handling for simultaneously providing -Ue & --upgrade-strategy may be misleading.

How to Reproduce
Installing/upgrading package in editable mod with:
pip install -Ue --upgrade-strategy eager .

Output
-e is eager for path/url argument, in addition to that, seems that there's name clutch for handling such case, output is:
ERROR: --upgrade-strategy should either be a path to a local project or a VCS url beginning with svn+, git+, hg+, or bzr+

@triage-new-issues triage-new-issues bot added the triage label Jun 25, 2019

@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Ah, you'd want to specify arguments/options differently. Right now, your invocation expands to:

pip install --upgrade --editable --upgrade-strategy eager .

However, editable expects an argument. A different invocation that will work:

pip install --upgrade --upgrade-strategy eager --editable .

Or even:

pip install -Ue . --upgrade-strategy eager

Cheers!

@3lnc

This comment has been minimized.

Copy link
Author

commented Jun 25, 2019

I know how it works and I know how to "work around". This is why I've noted the -e is eager for path param.

The issue is error message — problem with such invocation is definitely not "upgrade-strategy should either be a path to a local project or a VCS url".

Am I wrong here?

@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Am I wrong here?

@3lnc No; I misunderstood what you were saying the first time. Thanks for the clarification. :)

Changing the language to {} is not a valid editable requirement, it should either be a path to a local project or a VCS URL (beginning with svn+, git+, hg+, or bzr+). will resolve that.


This issue is a good starting point for anyone who wants to help out with pip's development -- it's simple and the process of fixing this should be a good introduction to pip's development workflow.

@enigma0160

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

Hi @pradyunsg , newbie here, I would like to take this up as my first issue!

Is it okay to simply test like so in constructors.py within the parse_editable() function?

if editable_req == '--upgrade-strategy':
        raise InstallationError(
            '{} is not a valid editable requirement '
            'it should either be a path to a local project or a VCS URL '
            '(beginning with svn+, git+, hg+, or bzr+).'
        )

But it seems a bit messy, should I try to change how the arguments are parsed? I am not sure how though. Any guidance would be helpful!

@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Hi @enigma0160! No worries!

We don't want to special case --upgrade-strategy here. It'll be better to change the site where we're raising the error from currently, to use the newer language I suggested above.

Feel free to ask me if you have any questions. :)

@enigma0160

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

So the correction I suggested before is fine? That is checking if editable_req == '--upgrade-strategy':
Or should I do something else?

@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

So the correction I suggested before is fine?

No.

should I do something else?

Yes. We'd want to change the following error:

raise InstallationError(
'%s should either be a path to a local project or a VCS url '
'beginning with svn+, git+, hg+, or bzr+' %
editable_req
)

And change the argument passed to the error to be equivalent to:

'{} is not a valid editable requirement it should either be a path to a local project or a VCS URL (beginning with svn+, git+, hg+, or bzr+).'.format(editable_req)

Note that, above, I'm using str.format method, which is IMO preferable to the % interpolation that's being done in constructors.py currently (it's newer and simpler). You can find more details about string interpolation in Python here: https://pyformat.info/.

The way we're transitioning between the two formats is, when we make other improvements to that bit of code (like change the error message here), we also change the syntax. That way we avoid the churn caused by a full-codebase change to switch between the two styles, since this doesn't add any value.

@enigma0160

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

Oh yeah, sorry, didn`t think of that!
So this should do right?

if '+' not in url:
        raise InstallationError(
            '{} is not a valid editable requirement. '
            'It should either be a path to a local project or a VCS URL '
            '(beginning with svn+, git+, hg+, or bzr+).'.format(editable_req)
        )
@enigma0160

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

I was reading through the contributors manual, what kind of a NEWS file should I create for this?
Is .trivial okay?

@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

So this should do right?

Yep.

reg: NEWS fragment

I'd say name it ".bugfix". Thus, you'd want to add a 6648.bugfix.

Improve error message printed when an invalid editable requirement is provided.
@enigma0160

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Okay and thanks a lot for all the help and for putting up with my silly questions!

@enigma0160

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Looks like the tests are failing because of the change in the phrasing.
Should I update the tests to reflect the changes we have made?
Or should I modify my commit?

@enigma0160

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@pradyunsg Is there anything else that I need to do in this, just wondering since its been a couple of days.

@lock lock bot added the S: auto-locked label Aug 10, 2019

@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.