Canonicalize req name while doing pre-install package search

[deveshks]

Fixes and closes #5021

Package name should be normalized before we use it to search if it's already installed or not. This will avoid cases like for e.g. pip install fluent.logger running install again, if pip install fluent-logger was already run, by ensuring that fluent.logger is normalized to fluent-logger

[deveshks]

Hi @uranusjr

I have filed this PR according to your suggestion on the ticket.

Also to write a unit test for this, is it enough to try and install two packages, one with a - and one with a ., and ensuring the second install doesn't go through? Or do we want to test some other variants, or some other commands as well with this?

[uranusjr]

I think you can just write a test to install one package, but pytest.mark.parametrize it to pip install with either dash, underscore, and dot. Those three should have identical output if this works.

[deveshks]

I think you can just write a test to install one package, but pytest.mark.parametrize it to pip install with either dash, underscore, and dot. Those three should have identical output if this works.

Thanks, I have parametrized the tests in the latest commit

[pradyunsg]

CI isn't particularly happy with this change. :)

[deveshks]

CI isn't particularly happy with this change. :)

Yep, I am having some difficulties writing a custom get_distribution, as sometimes the working_set itself increases in size as per #8054 (comment) 😞 . I am trying to work around that.

[pradyunsg]

LGTM, barring the clarification requested by @uranusjr.

[deveshks]

A very gentle nudge @uranusjr.

Thanks, I think this is ready to be merged.

[uranusjr]

The difference between search_distribution and get_distribution is a bit too subtle for me, it’d probably be a good idea to change them to something like get_distribution(name) and get_distribution(name, refresh=True) or something similar (I’m terribly at naming arguments, feel free to call the flag something else).

The implementation looks good to me though, so I wouldn’t mind this being kept as-is if others find it easy to read, or do a follow-up refactoring instead.

[deveshks]

The difference between search_distribution and get_distribution is a bit too subtle for me, it’d probably be a good idea to change them to something like get_distribution(name) and get_distribution(name, refresh=True) or something similar (I’m terribly at naming arguments, feel free to call the flag something else).

The implementation looks good to me though, so I wouldn’t mind this being kept as-is if others find it easy to read, or do a follow-up refactoring instead.

I thought of search_distribution to be used for looking up the package in the WorkingSet, and get_distribution to return that looked up package as a distribution.

I agree that we can address this in a follow-up refactoring as well, but I am okay to rename the function as well. I'll wait for @pradyunsg to give his thoughts on this as well, before deciding on either of them.

[deveshks]

Given that I can address the follow-up comment by @uranusjr in a separate PR, may I get this PR merged?

[deveshks]

Hi @pradyunsg

May I get this PR merged?

[deveshks]

Rebased to remove the extra merge commits.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[chrahunt]

One minor comment, otherwise this LGTM

[deveshks]

Thanks for the review @chrahunt . I have addressed the comment, and the CI is green now, so this can be merged :)