Skip to content
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
Merged

Conversation

mindw
Copy link
Contributor

@mindw mindw commented Aug 13, 2016

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

@jaraco
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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!

@jaraco
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants