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

Add is_yanked to installation report #12224

Merged
merged 5 commits into from
Sep 5, 2023
Merged

Conversation

ddelange
Copy link
Contributor

@ddelange ddelange commented Aug 14, 2023

This PR adds is_yanked to the installation report as a machine-readable alternative to #12225

@ddelange
Copy link
Contributor Author

Hi @pfmoore 👋 anything else from your side?

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

Thanks for this contrib! Code looks good to me. Could you update the documentation page too?

@@ -23,6 +23,9 @@ def _install_req_to_dict(cls, ireq: InstallRequirement) -> Dict[str, Any]:
# includes editable requirements), and false if the requirement was
# downloaded from a PEP 503 index or --find-links.
"is_direct": ireq.is_direct,
# is_yanked is true if the requirement was yanked from the index, but
# was still selected by pip to conform to PEP 592.
"is_yanked": ireq.link.is_yanked if ireq.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.

Do we know in which circumstances ireq.link can be None ?

Perhaps we should output "is_yanked": None in that case, to express that we don't know if it's yanked or not, rather the False which asserts it is not yanked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when ireq.link is None, it's not coming from a pypi index. since the notion of yanking only applies to pypi indices, I think returning False here is safe. but please correct me if I'm wrong @pfmoore

Copy link
Member

Choose a reason for hiding this comment

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

when ireq.link is None, it's not coming from a pypi index

If that's the reason of ireq.link is None, then it's fine to return is_yanked=False in such case, indeed.

@ddelange
Copy link
Contributor Author

ddelange commented Aug 24, 2023

Hi @sbidoul 👋 it looks like those docs are a bit outdated (e.g. is_direct is missing and instead a nested direct_url is there) - would that call for a general overhaul of that page, independent of this PR?

@sbidoul
Copy link
Member

sbidoul commented Aug 24, 2023

Hi @sbidoul 👋 it looks like those docs are a bit outdated (e.g. is_direct is missing and instead a nested direct_url is there) - would that call for a general overhaul of that page, independent of this PR?

Sorry I had linked the inspection report instead of the installation report (link now fixed).

AFAIK the installation report reference page is up-to-date. Even if it was not, that should not be a reason to let it drift even more :)

@ddelange
Copy link
Contributor Author

@sbidoul done 🤙

@@ -56,6 +56,9 @@ package with the following properties:
URL reference. `false` if the requirements was provided as a name and version
specifier.

- `is_yanked`: `true` if the requirement was yanked from the index, but was still
selected by pip conform to [PEP 592](https://peps.python.org/pep-0592/#installers).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
selected by pip conform to [PEP 592](https://peps.python.org/pep-0592/#installers).
selected by pip in comformance with [PEP 592](https://peps.python.org/pep-0592/#installers).

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im no native, but looks like 'conform to' and 'conform with' are both fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all four seem to occur, conform to seems most common on ngrams

Copy link
Member

Choose a reason for hiding this comment

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

Not native either :)

@uranusjr uranusjr merged commit 6328294 into pypa:main Sep 5, 2023
24 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants