Fix pip show displaying the incorrect result for "Required-by" #6957

ofrinevo · 2019-08-31T18:58:04Z

Fixes #6947

Hello, this is my first PR here so I hope I did everything properly.
I chose to have a separate test and stub because:

I needed a new test to check a Capitalized name.
I didn't want to change the existing stub since it is being used elsewhere.
I didn't want to use the same test because the two installs seemed too cluttered for one test.
I hope this is good as is, but if not, please say so and I'll change it to your liking.

Add test to check if a capitalized name is being shown properly Add new stub package to support the test

…o long

twice

chrahunt

Thanks for the PR! I think you did everything fine, it looks like all the checks are happy.

I have one minor comment that I have made on the code itself.

In addition, can we also add a test to check that a package (A, for example) that requires Requires_Capitalized as requires_Capitalized causes Required-by: A to appear in the output of pip show for Requires-Capitalized?

Just a note: In general we prefer to use unit tests where possible since those are usually faster (and we're trying to speed up our tests). In this case it would require some refactoring in commands/show.py so in my opinion these tests are OK and we can probably worry about it later.

tests/data/src/requires_capitalized/setup.py

in pip show Remove useless import in stubs for named tests

chrahunt · 2019-09-01T18:42:08Z

tests/data/src/required_by_mixed_capitalization/setup.py

@@ -0,0 +1,6 @@
+from setuptools import setup
+
+setup(name='requires_Capitalized',


Sorry, in hindsight my previous suggestion was not very clear.

I think that this package should be named "A" (or anything else, it doesn't matter), and then it should have requires_Capitalized in its install_requires list. Then we should check that the Requires_Capitalized package correctly puts "A" in its "Required-By" list. That would ensure that the names of things in install_requires that we're comparing against are also normalized.

I see.
As you can see by the following commits, I messed it up a bit.
Anyway, do you prefer having both the existing test and the new one? Just the new one?

No worries! I would keep test_show_required_by_packages_capitalized, remove test_show_required_by_with_mixed_capitalization, and add the new one (maybe called test_show_required_by_packages_requiring_capitalized).

I tried to do that earlier, but it didn't install the package because of:
ERROR: No matching distribution found for requires_Capitalized==1.0 (from simple-1==1.0)
(obviously)
So I figured I should Create a new tar.gz called Requires_Capitalized for this test.
I wanted to make sure with you first that this is in fact what you meant before I do so.

I think it would be OK to explicitly install Requires_Capitalized before installing A, then we should not need another .tar.gz.

Are you sure that installing it explicitly would work?
I've done the following:
(Don't mind the name, WIP and stuff)

Added a file

setup(name='Req_Cap_Todo', version='1.0', )

Installing this file:

editable_path = os.path.join(data.src, 'Req_Cap_Todo') script.pip( 'install', '--no-index', '-f', data.find_links, editable_path )

Installed the package

setup(name='simpleone', version='1.0', install_requires=['Req_Cap_Todo==1.0'] )

And I get the result of
['Name: Req-Cap-Todo', 'Version: 1.0', ... , 'Requires: ', 'Required-by: ']

To make sure, I tried to installed Click first and black afterwards, but it did show the correct message (with my fix, of course)

Installing explicitly was meant to avoid making another .tar.gz, and that looks like it worked. 👍 The other item is a bug.

chrahunt · 2019-09-01T21:22:23Z

tests/data/src/required_by_mixed_capitalization/setup.py

@@ -0,0 +1,6 @@
+from setuptools import setup
+
+setup(name='requires_Capitalized',


Installing explicitly was meant to avoid making another .tar.gz, and that looks like it worked. 👍 The other item is a bug.

chrahunt · 2019-09-01T21:22:49Z

src/pip/_internal/commands/show.py

        required_by = [
            pkg.project_name for pkg in pkg_resources.working_set
-            if name in [required.name for required in pkg.requires()]
+            if canonical_name in [required.name for required in pkg.requires()]


I think we should probably be doing canonicalize_name(required.name) here, that may fix the issue you mentioned with the "Required-by" of "Req-Cap-Todo" missing "simpleone" in our new test.

You are indeed correct, I really need to setup up my pycharm debugger with tox.
Everything passes now.

I also think we should extract this list comprehension to a method, under the rule of "a function should do one thing and one thing only".
I would've done so in a personal project (without thinking too much about it) but since I'm new here I'm checking with you.
Or better yet, should I put it in search_packages_info? (this methodneeds a refactor imo but that's a different story)

I agree this could be improved, and moving it into search_packages_info seems like it would work and make more sense.

If you make the change in a separate commit then it should be OK here. It may take a little longer for someone else to review and merge, or maybe not - everyone is a volunteer so it can vary wildly. A few things that could help:

If you squash/rewrite your commits into meaningful "chunks" of changes that have clear diffs (maybe one commit for the primary change here and then one for moving the requires_by calculation into search_packages_info) that would make it easier to evaluate this PR

Give each of those commits a clear message (reduces noise compared to having a handful of commits)

These aren't required by any means, but can reduce the work required to review and can be useful to know how to do.

Just FYI, not applicable here - in general if the primary change being made is larger or more complex I would probably do refactoring in separate PRs before the "main" one if I think they are required to make the "main" changes clear. If the refactoring is more like a nice-to-have but doesn't really improve understanding the "main" change then I would probably do it in a followup PR after the "main" one. You don't have to figure this out ahead of time on your own - if some refactoring is needed then it's OK to put the "main" PR on hold, do refactoring in a separate PR, and then rebase and continue with the "main" one when the refactoring is merged. Similarly it's OK to say in a PR comment or issue that some refactoring followup is needed.

Thank you very much for all the feedback, I appreciate it greatly.
I've been working in a python team for about 3 years but open source feedback adds much more.

Just one more question before I push:
I have an option to add a private:
def _get_requiring_packages(package_name):

return [ pkg.project_name for pkg in pkg_resources.working_set if canonicalize_name(package_name) in [canonicalize_name(required.name) for required in pkg.requires()] ]

or to do it directly in search_packages_info:

package = { 'name': dist.project_name, 'version': dist.version, 'location': dist.location, 'requires': [dep.project_name for dep in dist.requires()], 'required_by': [ pkg.project_name for pkg in pkg_resources.working_set if canonicalize_name(dist.project_name) in [canonicalize_name(required.name) for required in pkg.requires()] ] }

I feel like the first option is better (and passes the 79 char per line limit!) but again, wanted to make sure before I push.

1 seems better to me, but I would make it a free function in search_packages_info and drop the leading underscore (because its scope will already prevent it from being used outside, marking it private would be redundant IMO).

chrahunt

Looks good! Just a few minor comments.

chrahunt · 2019-09-03T00:22:12Z

src/pip/_internal/commands/show.py

+    def get_requiring_packages(package_name):
+        return [
+            pkg.project_name for pkg in pkg_resources.working_set
+            if canonicalize_name(package_name) in


I would assign canonicalize_name(package_name) outside the list comprehension and then use the variable inside, which looks a little simpler (also avoids recomputing every time).

chrahunt · 2019-09-03T00:23:07Z

tests/data/src/required_by_capitalized/required_by_capitalized/__init__.py

@@ -0,0 +1 @@
+#


We should be able to get rid of this file.

chrahunt · 2019-09-03T00:23:16Z

tests/data/src/required_by_mixed_capitalization/required_by_mixed_capitalization/__init__.py

@@ -0,0 +1 @@
+#


This one too.

chrahunt · 2019-09-03T00:23:22Z

tests/data/src/requires_capitalized/requires_capitalized/__init__.py

@@ -0,0 +1 @@
+#


And this one.

chrahunt · 2019-09-03T00:27:52Z

tests/data/src/required_by_mixed_capitalization/setup.py

+
+setup(name='simple',
+      version='1.0',
+      install_requires=['required_by_Capitalized==1.0']


I would call this package requires_requires_capitalized (impacting the name argument here and the directory name) and have it contain 'requires_Capitalized' in its install_requires, then we can remove required_by_Capitalized. There will be a few corresponding changes in test_show_required_by_packages_requiring_capitalized.

Rename stub package required_by_mixed_cap to requires_requires_cap Move the canonicalize_name calculation outside of a loop

chrahunt

LGTM. Thanks!

pfmoore · 2019-09-04T07:45:13Z

Thanks @ofrinevo for the contribution, and @chrahunt for the reviews!

pradyunsg · 2019-09-04T10:24:05Z

Great! Thanks for the contribution @ofrinevo (and @chrahunt and @pfmoore)!

@ofrinevo Do let us know if you're interested in contributing further. :)

ofrinevo · 2019-09-04T20:46:26Z

@pradyunsg Indeed I do.
I'll have some spare time in the next few months and Pip seems like a good place to spend it.

ofrinevo added 2 commits August 31, 2019 21:44

Fix package name not being canonicalize in commands/show

c674fab

Add test to check if a capitalized name is being shown properly Add new stub package to support the test

Add news file

fa4fc6a

ofrinevo changed the title ~~Bugfix/pip show displaying the incorrect result for "Required-by"~~ Fix pip show displaying the incorrect result for "Required-by" Aug 31, 2019

ofrinevo added 3 commits August 31, 2019 22:09

Fix test_show_required_by_packages_capitalized documentation being to…

ce17cf9

…o long

Remove a print command left by mistake in the test

b951dd9

Extract the canonical_name from name itself instead of using dist.get

9b6c0ea

twice

chrahunt reviewed Sep 1, 2019

View reviewed changes

tests/data/src/requires_capitalized/setup.py Outdated Show resolved Hide resolved

ofrinevo added 2 commits September 1, 2019 21:28

Add a test that checks for mixed upper and lower case letters

a05de61

in pip show Remove useless import in stubs for named tests

Improve the documentation of the added tests

35c2161

chrahunt reviewed Sep 1, 2019

View reviewed changes

ofrinevo added 2 commits September 2, 2019 19:41

Fix all failing tests

bc65d55

Move required_by calculations from print_results to search_packages_info

6da50d0

chrahunt reviewed Sep 3, 2019

View reviewed changes

Remove useless python packages from required_cap

1c48811

Rename stub package required_by_mixed_cap to requires_requires_cap Move the canonicalize_name calculation outside of a loop

chrahunt approved these changes Sep 3, 2019

View reviewed changes

pfmoore merged commit bc78bcd into pypa:master Sep 4, 2019

lock bot added the auto-locked Outdated issues that have been locked by automation label Oct 4, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pip show displaying the incorrect result for "Required-by" #6957

Fix pip show displaying the incorrect result for "Required-by" #6957

ofrinevo commented Aug 31, 2019

chrahunt left a comment •

edited

Loading

chrahunt Sep 1, 2019 •

edited

Loading

ofrinevo Sep 1, 2019

chrahunt Sep 1, 2019 •

edited

Loading

ofrinevo Sep 1, 2019

chrahunt Sep 1, 2019

ofrinevo Sep 1, 2019

chrahunt Sep 1, 2019

chrahunt Sep 1, 2019

chrahunt Sep 1, 2019

ofrinevo Sep 2, 2019 •

edited

Loading

chrahunt Sep 2, 2019 •

edited

Loading

ofrinevo Sep 2, 2019

chrahunt Sep 2, 2019

chrahunt left a comment

chrahunt Sep 3, 2019

chrahunt Sep 3, 2019

chrahunt Sep 3, 2019

chrahunt Sep 3, 2019

chrahunt Sep 3, 2019

chrahunt left a comment

pfmoore commented Sep 4, 2019

pradyunsg commented Sep 4, 2019

ofrinevo commented Sep 4, 2019

		@@ -0,0 +1,6 @@
		from setuptools import setup

		setup(name='requires_Capitalized',

Fix pip show displaying the incorrect result for "Required-by" #6957

Fix pip show displaying the incorrect result for "Required-by" #6957

Conversation

ofrinevo commented Aug 31, 2019

chrahunt left a comment • edited Loading

Choose a reason for hiding this comment

chrahunt Sep 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrahunt Sep 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofrinevo Sep 2, 2019 • edited Loading

Choose a reason for hiding this comment

chrahunt Sep 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrahunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrahunt left a comment

Choose a reason for hiding this comment

pfmoore commented Sep 4, 2019

pradyunsg commented Sep 4, 2019

ofrinevo commented Sep 4, 2019

chrahunt left a comment •

edited

Loading

chrahunt Sep 1, 2019 •

edited

Loading

chrahunt Sep 1, 2019 •

edited

Loading

ofrinevo Sep 2, 2019 •

edited

Loading

chrahunt Sep 2, 2019 •

edited

Loading