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

Filter out yanked links from available versions error message #12225

Merged
merged 1 commit into from Aug 28, 2023

Conversation

ddelange
Copy link
Contributor

Fixes #11745

@uranusjr
Copy link
Member

I wonder if this would introduce confusion from the other direction, i.e. pip must have a bug to not find a version that’s listed on PyPI. It may be a good idea to still show the yanked versions, either separately or annotated.

@ddelange
Copy link
Contributor Author

@uranusjr I added a conditional logger.critical mentioning yanked versions if any

@ddelange ddelange force-pushed the filter-available-versions branch 4 times, most recently from 66a5ef2 to ad8da8b Compare August 14, 2023 19:26
Comment on lines 616 to 621
yanked_versions = [
str(v)
for v in sorted(
{c.version for c in cands if (c.link.is_yanked if c.link else False)}
)
]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can compare len(versions) and len(cands) and skip building this list if they match (indicating there’re no yanked versions at all)

Copy link
Member

Choose a reason for hiding this comment

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

Also I kind of wonder if the yanked version can be info instead. That’s just minor and subjective though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I understood @pfmoore correctly, a version can be in both lists simultaneously if only some of the links for this version are yanked.

nothing to worry about for PyPI, but maybe this can be encountered elsewhere in the wild?

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I’ll accept this as-is and someone else can worry about optimisation later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An equivalent single-lookup variation:

is_yanked = {True: set(), False: set()}
for c in cands:
    is_yanked[c.link.is_yanked if c.link else False].add(c.version)

versions = sorted(is_yanked[False])
yanked_versions = sorted(is_yanked[True])

How about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @uranusjr @pfmoore 👋

I pushed the snippet above. Anything else from my side?

@ddelange ddelange force-pushed the filter-available-versions branch 4 times, most recently from bf63604 to 711c4b8 Compare August 23, 2023 11:35

is_yanked: Dict[bool, Set[CandidateVersion]] = {True: set(), False: set()}
for c in cands:
is_yanked[c.link.is_yanked if c.link else False].add(c.version)
Copy link
Contributor Author

@ddelange ddelange Aug 24, 2023

Choose a reason for hiding this comment

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

@sbidoul the same reasoning from #12224 (comment) would apply here

is_yanked[c.link.is_yanked if c.link else False].add(c.version)

versions = sorted(str(v) for v in is_yanked[False])
yanked_versions = sorted(str(v) for v in is_yanked[True])
Copy link
Member

Choose a reason for hiding this comment

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

We should sort the versions, then convert to str for the sorting to be accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch! could you have a look again?


is_yanked: Dict[bool, Set[CandidateVersion]] = {True: set(), False: set()}
Copy link
Member

Choose a reason for hiding this comment

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

This dict feels a little bit awkward to me. I'd have done sets and a good old if. It might be a matter of personal preference though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, removed 👍

@uranusjr uranusjr merged commit a337816 into pypa:main Aug 28, 2023
26 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants