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

Remove unwanted annotations from primary packages #542

Conversation

quantus
Copy link
Contributor

@quantus quantus commented Jul 10, 2017

primary_packages contains lower case package names where as ireq.name may contain upper case letters. This causes the primary packages to have the unwanted # via XXX annotations. In my case this issue was seen with packages SQLAlchemy and Flask.

Contributor checklist
  • Provided the tests for the changes
  • Added the changes to CHANGELOG.md
  • Requested (or received) a review from another contributor

@quantus quantus force-pushed the hotfix/do-not-annotate-capitalized-primary-packages branch from 84a87ba to 3e2b62a Compare July 11, 2017 14:58
@quantus
Copy link
Contributor Author

quantus commented Jul 19, 2017

I forgot to tell in the PR how this problem can be reproduced.

Setup:

mkvirtualenv --python=$(which python2.7) test 
pip install Flask Flask-Debugtoolbar pip-tools
echo "Flask" > requirements.in
echo "Flask-Debugtoolbar" >> requirements.in

After the setup:

(test) ~/Documents/tmp pip-compile requirements.in 
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file requirements.txt requirements.in
#
blinker==1.4              # via flask-debugtoolbar
click==6.7                # via flask
flask-debugtoolbar==0.10.1
flask==0.12.2             # via flask-debugtoolbar
itsdangerous==0.24        # via flask, flask-debugtoolbar
jinja2==2.9.6             # via flask
markupsafe==1.0           # via jinja2
werkzeug==0.12.2          # via flask, flask-debugtoolbar

Version info:

(test) ~/Documents/tmp pip-compile --version
pip-compile, version 1.9.0
(test) ~/Documents/tmp pip --version
pip 9.0.1 from /Users/pekka/.virtualenvs/test/lib/python2.7/site-packages (python 2.7)

As we can see from the output, the flask dependency was listed to come from # via flask-debugtoolbar. As the flask dependency was our primary dependency, that comment should not be there.

With this information I feel this PR is ready to be reviewed.

Copy link
Member

@vphilippon vphilippon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you.
I requested a small change, other than that, LGTM

@@ -137,7 +137,7 @@ def _format_requirement(self, ireq, reverse_dependencies, primary_packages, mark
for hash_ in sorted(ireq_hashes):
line += " \\\n --hash={}".format(hash_)

if not self.annotate or ireq.name in primary_packages:
if not self.annotate or ireq.name.lower() in primary_packages:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch
While at it, as primary_packages was generated using key_from_req, we should use that here too for consistency.
key_from_req(ireq.req) in primary_packages

@quantus quantus force-pushed the hotfix/do-not-annotate-capitalized-primary-packages branch from 3e2b62a to 38f8d61 Compare August 21, 2017 11:05
@quantus quantus force-pushed the hotfix/do-not-annotate-capitalized-primary-packages branch from 38f8d61 to 1e7076c Compare August 21, 2017 11:08
@quantus
Copy link
Contributor Author

quantus commented Aug 21, 2017

@vphilippon: I've made the change you requested.

@vphilippon vphilippon added this to the 1.10 milestone Aug 21, 2017
@vphilippon
Copy link
Member

Thanks for the contribution!

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