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

requirements file parsing with normal option defaults #2725

Merged
merged 5 commits into from Apr 28, 2015

Conversation

Projects
None yet
3 participants
@qwcode
Contributor

qwcode commented Apr 25, 2015

  1. parse with defaults set as they are (vs forcing to None) and adjust the logic to match; the result is simpler.
  2. Due to #1, we can remove some hairy "format_control" hacks that arose in response.
  3. Due to #1, let's relax the parsing and allow:
    • multiple options per line
    • any supported option on a line with a requirement (not just --install-option/--global-option, although they are the only options that are scoped to the requirement)
      #3 may be debatable, but it seems ok to me.

cc @rbtcollins , since you had to work around this for format_control parsing

qwcode added some commits Apr 25, 2015

1) parse with defaults set as they are naturally (vs forcing to None)
   and adjust the logic to match; the result is simpler.
2) Due to #1, we can remove some hairy "format_control" hacks
3) Due to #1, we have to relax the parsing and allow:
   - multiple options per line
   - any supported option on a line with a requirement (not just
     --install-option/--global-option, although they are the only
     options that are passed into a requirement)
@rbtcollins

This comment has been minimized.

Show comment
Hide comment
@rbtcollins

rbtcollins Apr 25, 2015

Contributor

Looks much cleaner to me.

Contributor

rbtcollins commented Apr 25, 2015

Looks much cleaner to me.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 28, 2015

Contributor

@dstufft @pfmoore, can I get your thoughts on #3 from the description, since it's somewhat major.

we could check when option values are != to the defaults, but it's only a partial solution. we'd still fail to detect when:

  • an option happens to set the default value
  • when there's multiple of the same option

We could possibly do some low-level overrides in optparse to detect this, but it seems like the wrong path.

The important thing is that requirements can still only be one per line, and --install-option/--global-option on those lines are scoped to the requirement.

Contributor

qwcode commented Apr 28, 2015

@dstufft @pfmoore, can I get your thoughts on #3 from the description, since it's somewhat major.

we could check when option values are != to the defaults, but it's only a partial solution. we'd still fail to detect when:

  • an option happens to set the default value
  • when there's multiple of the same option

We could possibly do some low-level overrides in optparse to detect this, but it seems like the wrong path.

The important thing is that requirements can still only be one per line, and --install-option/--global-option on those lines are scoped to the requirement.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Apr 28, 2015

Member

#3 seems OK to me. It mostly allows things that I don't think people will (or should) ever do.

I'd assume that the extra behaviour would be undocumented. So we have the option to treat weird behaviour as "don't do that" rather than as bugs to fix. I'd prefer that over trying to document and support all the corner cases.

Member

pfmoore commented Apr 28, 2015

#3 seems OK to me. It mostly allows things that I don't think people will (or should) ever do.

I'd assume that the extra behaviour would be undocumented. So we have the option to treat weird behaviour as "don't do that" rather than as bugs to fix. I'd prefer that over trying to document and support all the corner cases.

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

Merge pull request #2725 from qwcode/req_file_parse_defaults
requirements file parsing with normal option defaults

@qwcode qwcode merged commit 94a6829 into pypa:develop Apr 28, 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