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

Refactor requirements file parsing #2697

Merged
merged 15 commits into from Apr 23, 2015

Conversation

Projects
None yet
3 participants
@qwcode
Contributor

qwcode commented Apr 18, 2015

Two major changes:

  1. Re-use the optparse options in pip.cmdoptions and build a standard optparse parser, instead of maintaining a custom parser. This improves consistency and reduces bugs when we make options changes, and should make the eventual click-rehaul of our cli easier.
  2. As a result of #1, the call stack is simpler:
    • from: parse_requirements -> parse_content -> parse_line
    • to: parse_requirements -> process_line

Changes beyond 1 & 2:

  • Adjusting the tests to match
  • Adding more tests (specifically testing every supported option, and functional test that confirms #2537)
  • Minor cosmetics

There are some behavior changes:

  • The parsing is now consistent with the cli parsing. For example, before we were allowing things like "-i=url", and possibly other subtle differences.
  • We now allow multiple of the same option (but not different options) on the same line, largely due to not having an easy way to detect this using optparse. Is there a way?

qwcode added some commits Apr 17, 2015

refactor the requirements file parsing
two major changes:

1) re-use the optparse options in pip.cmdoptions instead of maintaining
   a custom parser

2) as a result of #1, simplify the call stack
    from:  parse_requirements -> parse_content -> parse_line
      to:  parse_requirements -> process_line

beyond #1/#2, minor cosmetics and adjusting the tests to match
- fixes to `TestOptionVarants`
- consistent use of the finder fixture
- inline the logic from the get_options_dest function
- report the option string when using an incorrect option with a requirement
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 21, 2015

Contributor

@pfmoore thoughts? since you looked at this code recently?

Contributor

qwcode commented Apr 21, 2015

@pfmoore thoughts? since you looked at this code recently?

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Apr 21, 2015

Member

I've had a quick look. I don't have much time over the next few days to do a full review, but the new code looks a lot clearer than the previous version, which is great. Processing looks fine.

There's a lot of changes to the tests, which I haven't looked at in detail but which seem to be mostly because they are testing internal functions of the old code, and so are no longer applicable. I'm not sure to what extent (either before or after this patch) we test the "higher level" aspects of requirement file processing (i.e. for various requirement files, does the parsing give the overall results that are expected), but that's not directly relevant to this patch - it may be useful to review as part of the more general "improve our tests" process. But for this patch, I'm assuming (for now) that test coverage remains sufficient.

I'll try to get time to do a more thorough review, but it might not be for a couple of weeks, so don't wait on me. Overall, though, the change looks good to me.

Member

pfmoore commented Apr 21, 2015

I've had a quick look. I don't have much time over the next few days to do a full review, but the new code looks a lot clearer than the previous version, which is great. Processing looks fine.

There's a lot of changes to the tests, which I haven't looked at in detail but which seem to be mostly because they are testing internal functions of the old code, and so are no longer applicable. I'm not sure to what extent (either before or after this patch) we test the "higher level" aspects of requirement file processing (i.e. for various requirement files, does the parsing give the overall results that are expected), but that's not directly relevant to this patch - it may be useful to review as part of the more general "improve our tests" process. But for this patch, I'm assuming (for now) that test coverage remains sufficient.

I'll try to get time to do a more thorough review, but it might not be for a couple of weeks, so don't wait on me. Overall, though, the change looks good to me.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 21, 2015

Contributor

thanks @pfmoore . I think the tests are overall improved in this PR. Specifically, the functional test now confirms the override behavior for real using "--prefix", i.e. it confirms a prefix value in the req file overrides the value in the cli. Also, there are now tests for every supported option.

thoughts @gvalkov ?

Contributor

qwcode commented Apr 21, 2015

thanks @pfmoore . I think the tests are overall improved in this PR. Specifically, the functional test now confirms the override behavior for real using "--prefix", i.e. it confirms a prefix value in the req file overrides the value in the cli. Also, there are now tests for every supported option.

thoughts @gvalkov ?

Show outdated Hide outdated pip/req/req_file.py Outdated
Show outdated Hide outdated pip/req/req_file.py Outdated
Show outdated Hide outdated pip/req/req_file.py Outdated
@gvalkov

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Apr 21, 2015

Contributor

I really like this - it's definitely an improvement/simplification over the previous implementation.

Using spaces in the requirement specifier is allowed, right? I think I use it like that all the time (i.e pillow >= 2.8.1). This PR would mandate that all specifiers single words (i.e pillow>=2.8.1). It's surprising that there wasn't a test to catch this.

Happy to hear that pip is considering click for its command-line interface - I find the sub-parser handling in it nicely done, but I'm not too excited about functions with up to 20 arguments (as would be the case for the install command).

Contributor

gvalkov commented Apr 21, 2015

I really like this - it's definitely an improvement/simplification over the previous implementation.

Using spaces in the requirement specifier is allowed, right? I think I use it like that all the time (i.e pillow >= 2.8.1). This PR would mandate that all specifiers single words (i.e pillow>=2.8.1). It's surprising that there wasn't a test to catch this.

Happy to hear that pip is considering click for its command-line interface - I find the sub-parser handling in it nicely done, but I'm not too excited about functions with up to 20 arguments (as would be the case for the install command).

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 21, 2015

Contributor

Using spaces in the requirement specifier is allowed, right?

yes, the only caveat is the need to quote when using as cli arguments in the shell

Contributor

qwcode commented Apr 21, 2015

Using spaces in the requirement specifier is allowed, right?

yes, the only caveat is the need to quote when using as cli arguments in the shell

qwcode added a commit that referenced this pull request Apr 23, 2015

Merge pull request #2697 from qwcode/refactor_req_file
Refactor requirements file parsing

@qwcode qwcode merged commit 31eb67d into pypa:develop Apr 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

aapa added a commit to aapa/pyfibot that referenced this pull request May 31, 2015

From [pip 7.0.0 release notes](https://pip.pypa.io/en/latest/news.html):
> - **BACKWARD INCOMPATIBLE** Requirements in requirements files containing markers must now be quoted due to parser changes from ([PR #2697](pypa/pip#2697)) and ([PR #2725](pypa/pip#2725)). For example, use `"SomeProject; python_version < '2.7'"`, not simply `SomeProject; python_version < '2.7'`

aapa added a commit to aapa/pyfibot that referenced this pull request May 31, 2015

From [pip 7.0.0 release notes](https://pip.pypa.io/en/latest/news.html):
> - **BACKWARD INCOMPATIBLE** Requirements in requirements files containing markers must now be quoted due to parser changes from ([PR #2697](pypa/pip#2697) and ([PR #2725](pypa/pip#2725). For example, use `"SomeProject; python_version < '2.7'"`, not simply `SomeProject; python_version < '2.7'`

aapa added a commit to aapa/pyfibot that referenced this pull request May 31, 2015

Update requirements.txt to reflect syntax changes
From [pip 7.0.0 release notes](https://pip.pypa.io/en/latest/news.html):

> - **BACKWARD INCOMPATIBLE** Requirements in requirements files containing markers must now be quoted due to parser changes from ([PR #2697](pypa/pip#2697)) and ([PR #2725](pypa/pip#2725)). For example, use `"SomeProject; python_version < '2.7'"`, not simply `SomeProject; python_version < '2.7'`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment