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 PathDistribution._normalized_name implementation #377

Conversation

benoit-pierre
Copy link
Contributor

  • correctly handle paths with no embedded version (e.g. pkg.egg-info must yield the name pkg, not pkg.egg)
  • apply PEP 503 normalization to the extracted names (e.g.: zope..inter_face-4.2.dist-info must yield the name zope_inter_face)

Both are necessary, or entry_points(…) can yield the entry-points of a shadowed distribution. For example: with a version of mypkg in the system' site-packages directory when working from another development checkout of the same package (with a mypgk.egg-info directory mishandled by the first bug).

@jaraco
Copy link
Member

jaraco commented May 21, 2022

Thanks @benoit-pierre for this proposal. Unfortunately, I'm struggling a little bit with it, mainly because it fixes two unreported bugs in one commit. I'm also uneasy about the added complexity that "normalized names" is adding here (requiring fixtures with multiple return values). Would you mind writing up a bug report describing how you encountered this bug and how we might replicate it (and prove the fix)?

I'm a little disappointed in the test suite right now. It seems the performance tests aren't running. Also, the linter didn't catch that name was shadowed and never used, which might have been an indicator that something was wrong.

@jaraco
Copy link
Member

jaraco commented May 21, 2022

In #379, I've addressed the first issue.

@jaraco jaraco force-pushed the fix_PathDistribution._normalized_name_implementation branch 3 times, most recently from ad58bb0 to 1a1a3db Compare May 21, 2022 15:13
- apply PEP 503 normalization to the extracted names
  (e.g.: `zope..inter_face-4.2.dist-info` must yield
   the name `zope_inter_face`)

`entry_points(…)` can yield the entry-points
of a shadowed distribution. For example: with a version of `mypkg`
in the system' site-packages directory when working from another
development checkout of the same package (with a `mypkg.egg-info`
directory mishandled by the first bug).
@jaraco jaraco force-pushed the fix_PathDistribution._normalized_name_implementation branch from 1a1a3db to 596abd0 Compare May 21, 2022 15:15
assert version(pkg_name) == '1.0'
pkg_name, norm_pkg_name = self.pkg_with_dashes(self.site_dir)
dist = distribution(pkg_name)
assert dist._normalized_name == norm_pkg_name
Copy link
Member

Choose a reason for hiding this comment

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

Instead of testing the internal implementation details, I'd rather have a test that captures the failed expectation - namely that entry points of two packages whose file system names vary only by normalization won't deduplicate.

jaraco added a commit that referenced this pull request May 21, 2022
@jaraco
Copy link
Member

jaraco commented May 21, 2022

In #381, I've found I can capture the missed expectation by simply tweaking the test for test_entry_points_unique_packages to include the expected normalization.

@jaraco
Copy link
Member

jaraco commented May 21, 2022

These changes are superseded by #379 and #381. Thanks for the contrib.

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.

2 participants