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

fix for extra names containing '-' #732

Merged
merged 1 commit into from Oct 14, 2016

Conversation

Projects
None yet
2 participants
@mindw
Contributor

mindw commented Aug 13, 2016

This is variation of #578 with a more modest goal.

@jaraco

This comment has been minimized.

Member

jaraco commented Aug 13, 2016

Thanks for this and especially thanks for the test.

This change feels like it's shifting the spec for what is a valid extra name. A change like this probably should be reviewed by a larger audience. Has this been discussed elsewhere? Is there a reason that this shift is the obvious best choice?

Also, if/when we decide to move forward with this, would you include a CHANGES.rst entry describing for users the effect of the change?

@mindw

This comment has been minimized.

Contributor

mindw commented Aug 14, 2016

I'm a bit at a loss here - this looks to me like a pure bugfix:

  1. setuptools fails to record extras with '-' - such as faster-signatures.
  2. setuptools fails to find dependencies for faster-signatures.

Both items are required by pep508 and pep 426 (even though it's deferred). A good "in the wild" can be found at https://bitbucket.org/pypa/wheel/src/cf4e2d98ecb1f168c50a6de496959b4a10c6b122/setup.py?fileviewer=file-view-default .

@jaraco

This comment has been minimized.

Member

jaraco commented Aug 15, 2016

Oh, great. That PEP provides an excellent authoritative reference to indicate from whence the change is derived.

I see how from one perspective this feels like a bugfix, but from another perspective, the previous, established implementation seems to think that it will always normalize by replacing certain characters with underscores.

I wonder if accompanying this change if check_extras should be updated to restrict extra names to match the PEP requirements.

Still, if you want to leave that for others to address later, that's fine. Just add a CHANGES.txt entry for this change, referencing the PEP, and we're good.

@mindw mindw force-pushed the mindw:fix_extra_names_dash branch from ee643fc to 18c0a40 Aug 16, 2016

@mindw

This comment has been minimized.

Contributor

mindw commented Aug 20, 2016

As you correctly observed Foo[baz_lightyear] will fail lookup with this patch. It is a regression.

To move forward can you please assist me in figuring out what is the correct behavior of looking up extras as pep426 states that :

The names of extras MUST abide by the same restrictions as those for distribution names

and

All comparisons of distribution names MUST be case insensitive, and MUST consider hyphens and underscores to be equivalent.

while pep508 defines the extras as:

identifier_end = letterOrDigit | (('-' | '_' | '.' )* letterOrDigit)
identifier    = < letterOrDigit identifier_end* >
name          = identifier
extras_list   = identifier:i (wsp* ',' wsp* identifier)*:ids -> [i] + ids
extras        = '[' wsp* extras_list?:e wsp* ']' -> e

But makes not statement about how extras are looked up (AFAIK).

Thanks!

@mindw mindw force-pushed the mindw:fix_extra_names_dash branch from 18c0a40 to 8475f8b Aug 20, 2016

@mindw mindw force-pushed the mindw:fix_extra_names_dash branch from 8475f8b to 452e13c Aug 23, 2016

@jaraco

This comment has been minimized.

Member

jaraco commented Oct 14, 2016

can you please assist me in figuring out what is the correct behavior of looking up extras

I'm afraid I'm just not finding the time to dig into this. I'm inclined to just accept it and let subsequent requests improve on this implementation as necessary.

@jaraco jaraco merged commit 72cc77c into pypa:master Oct 14, 2016

2 checks passed

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

@bittner bittner referenced this pull request Nov 6, 2018

Open

Add metadata validation #147

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