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

Fixes #999: support python_requires and py_modules in configuration files #1007

Merged
merged 2 commits into from Apr 8, 2017

Conversation

Projects
None yet
4 participants
@mbargull
Contributor

mbargull commented Apr 7, 2017

Changes:

  • copy self.metadata.python_requires = self.python_requires patch to Distribution.parse_config_files
  • add 'py_modules': parse_list to ConfigOptionsHandler.parsers
  • add changes to docs and tests
  • fix error message in dist.check_specifier

Concerning the last point: packaging.specifiers.SpecifierSet.__init__ expects a str and no list. The fixed error is actually not thrown if a list is provided, since it fails on trying to call split(",") and thus will not throw a packaging.specifiers.InvalidSpecifier exception. If this case should be handled properly, it could be done here or rather in packaging.

@jaraco

This comment has been minimized.

Member

jaraco commented Apr 7, 2017

Looks good. Can you add one more commit with an entry to the changelog explaining the change to users? Probably an 0.1 bump.

@jaraco

This comment has been minimized.

Member

jaraco commented Apr 7, 2017

For future commits, I would recommend instead of committing all the changes in one commit and then explaining the changes in the PR, I'd prefer if you made several commits, each with a commit message explaining what you've explained above. The advantage to that approach is that in a year from now, if someone is tracing through the code history to understand why (for example) the error message was changed, they would readily find a description of the reason, rather than see what looks like a change incidental to the intention of the commit (and then being left to wonder if the change was intentional or incidental or accidental) and if they're lucky, tracing the commit message to the ticket to the PR for more detail. Separate commits also enables selective acceptance and rollback of independent changes.

@mbargull

This comment has been minimized.

Contributor

mbargull commented Apr 7, 2017

I fully agree. Sometimes (for whatever reason) project maintainers are averse to fine granular commits -- good to know you aren't!

@jaraco jaraco merged commit 4b664a6 into pypa:master Apr 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@idlesign

This comment has been minimized.

Member

idlesign commented Aug 10, 2017

@mbargull Hi, I wonder why python_requires is not parsed as a list to be consistent with other _requires?

@webknjaz

This comment has been minimized.

Contributor

webknjaz commented Aug 19, 2017

@idlesign it is rather an expression, than a list of stuff. This checks only one requirement — the version of runtime. I don't think there's machinery for handling and combining a list of such expressions.

@idlesign

This comment has been minimized.

Member

idlesign commented Aug 22, 2017

@webknjaz Nope, version specifier notation is desribed in PEP 345, and I think we'd have parsed it as a list for better user experience.

@mbargull

This comment has been minimized.

Contributor

mbargull commented Aug 29, 2017

Hi @idlesign, well I only followed setuptools' documentation:

python_requires
A string corresponding to a version specifier (as defined in PEP 440) for
the Python version, used to specify the Requires-Python defined in PEP 345.

And I'm with @webknjaz on this: In comparison to the other _requires fields, this only specifies the version(s) of one "kind of package" (=runtime) instead of multiple packages like the other fields. So what would be the benefit to make this a list of version specifiers? Do you have any legitimate use case in mind?

@idlesign

This comment has been minimized.

Member

idlesign commented Aug 30, 2017

So what would be the benefit to make this a list of version specifiers? Do you have any legitimate use case in mind?

I'll try to break it down. Let's think about it the following way: = 2.7, >= 3.3 is basically an equivalent to python = 2.7, python >= 3.3.

If so, we deal with the following two entities:

  1. python = 2.7
  2. python >= 3.3

The same as with:

  1. requests = 2.18
  2. requests >= 3.15

Requests 2.18 is the same as 3.15? Probably not, despite of the name we have two different applications. And we tokenize requests = 2.18, requests >= 3.15 into two tokens, breaking it on comma. It seems logically correct to have two items instead of one string representing both of them.

The idea is: if we have different entities they should be in different slots.
Why we have two entities? Because 2.7 is not the same as 3.3.

And if I use parse_configuration, say, for the purpose of gathering py2 and 3 stats,
I'd rather expect ['= 2.7', '>= 3.3'] instead of = 2.7, >= 3.3.

@webknjaz

This comment has been minimized.

Contributor

webknjaz commented Aug 30, 2017

@idlesign AFAIK separate entries like

requests == 2.18
requests >= 3.15

Would cause pip do install on 'em one by one and fail on the second entry, if it's

requests == 2.18, >= 3.15

then it would fail instantly when trying to install requirement with two incompatible restrictions.

Likewise python_requires >=2.7, !=3.0.*, != 3.1.*, !=3.2 is mentally expected to be similar.
I think that @mbargull and I are talking about semantics and you are thinking of the complexity of internally representing the constraint as some data structure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment