Adds a detection for index urls similar to how comments are captured. #825
Conversation
|
Here is a sample diff of a test I ran against a requirements file w/ comments and index urls |
tests/requirements_txt_fixer_test.py
Outdated
| b'# Foo\n' | ||
| b'-i http://dist.repoze.org/zope2/2.10/simple\n' | ||
| b'zopelib1\n' | ||
| b'# Bar\n' | ||
| b'--extra-index-url http://dist.repoze.org/zope2/2.10/simple\n' | ||
| b'zopelib2\n' | ||
| b'# Baz\n' | ||
| b'--index-url http://dist.repoze.org/zope2/2.10/simple\n' | ||
| b'zopelib3\n', |
There was a problem hiding this comment.
this isn't quite what I'd expect -- I think all options should stay at the top of the file
There was a problem hiding this comment.
I've updated my changes to push index-url options to the top and preserve proper order (index-url before extra-index-url). If this is sufficient, great, please review/approve/merge assuming all test pass.
As a point of discussion, I'm wondering if there is a better solution here. It is my understanding that the index url and/or extra index url can be specified before any requirement to change the indices used for that particular package. If my understanding is correct, and the options are all put at the top I think there might be a few other unhandled edge cases. Since it seems that the consensus is similar to what you originally suggested "never use --extra-index-url" I'm wondering if it would be better to do one of the following:
- push index-url to top and additionally fail + print an error if extra-index-url is detected or multiple index-url's are detected
- push all index-url options to top and leave extra-index-url associated with the specific requirement it appears above
- something else
There was a problem hiding this comment.
please review/approve/merge assuming all test pass.
yeah don't tell me what to do -- this is also just how things work so you don't need to say this
It is my understanding that the index url and/or extra index url can be specified before any requirement to change the indices used for that particular package
don't guess, figure out how it works -- that's a very important part of making this change especially if you're wrong
There was a problem hiding this comment.
yeah don't tell me what to do -- this is also just how things work so you don't need to say this
I wasn't trying to hence the please. I'll also try to keep things brief.
don't guess, figure out how it works -- that's a very important part of making this change especially if you're wrong
I'd recommend leaving it as it stand now (pushing all options to top).
I wasn't guessing but the pip documentation for requirements files also doesn't seem to cover the minutia of index-url and extra-index-url options. I know that users can change index-url throughout the file but it doesn't appear to be recommended best practice and neither is using extra-index-url. I raised this as a discussion point because I thought, based on the comments made to the test case, that you might have further insights to help me understand better.
There was a problem hiding this comment.
try it and figure out how it works and document it
…ed. This should avoid breaking ordering when --index-url (-i) and/or --extra-index-url are used. Also adds a relevant test to cover this case. Closes #612.
This should avoid breaking ordering when --index-url (-i) and/or --extra-index-url are used.
Closes #612.