Skip to content

Conversation

@jpoehnelt
Copy link

not sure if this has any other ramifications

@bazel-io
Copy link

Can one of the admins verify this patch?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@jpoehnelt jpoehnelt changed the title always lowercase always lowercase requirements dictionary keys Sep 20, 2017
@jpoehnelt
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@duggelz
Copy link

duggelz commented Sep 20, 2017

From PEP 508 it appears that you can register packages with any capitalization, and install them with any capitalization, even if it's different. I.e. you can register package 'MyPackage' and install it with mypackage, MYPACKAGE, or even mYpAcKaGe.

That is not an ideal spec, but is it the spec, and the tools implement it, so we should to.

@jpoehnelt jpoehnelt closed this Sep 20, 2017
@jpoehnelt jpoehnelt deleted the patch-1 branch September 20, 2017 01:20
@duggelz
Copy link

duggelz commented Sep 20, 2017

Patch looks ok to me, but we'll want to add a test (package PyYAML seems a good choice), and then an admin (@mattmoor) will need to rebuild the piptool.par file.

@duggelz
Copy link

duggelz commented Sep 20, 2017

Sorry, that was actually a "yes this seems like a good patch", not a "no". We should accept requirements("PyYAML") and normalize it to "pyyaml" like pip does.

@duggelz
Copy link

duggelz commented Sep 20, 2017

I guess the real test would be if some library had requirements("PyYAML") and some other library had requirements("pyyaml"). We would want to realize that those are actually the same thing.

@mattmoor
Copy link
Contributor

This LGTM, not sure why @JustinWP closed/deleted, but I'll recreate it with an updated PAR. thanks Justin.

alexeagle pushed a commit to alexeagle/rules_python that referenced this pull request Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants