Use strict optional checking in legacy resolver #12295
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The assert in _set_req_to_reinstall is definitely correct, the three call sites have guards The assert in _populate_link is a little trickier. It's not obvious to me how to prove it will never trigger. Note that if link were ever None, it could potentially cause AttributeError in Cache._get_cache_path_parts and direct_url_from_link The assert in _resolve_one is safe, if req_to_install.name was None it would raise TypeError in canonicalize_name I can't prove that req.name is not None in get_installation_order and the code would work fine if that were the case (since it's a defaultdict), so I opted to just ignore
1 task
chrysle
reviewed
Sep 24, 2023
| @@ -11,7 +11,6 @@ | |||
| """ | |||
|
|
|||
| # The following comment should be removed at some point in the future. | |||
There was a problem hiding this comment.
Suggested change
| # The following comment should be removed at some point in the future. |
uranusjr
reviewed
Sep 26, 2023
|
Since the legacy resolver is deprecated and slated for removal (at some point ™️), I don't believe it's worth the effort making it conform to a stricter mypy configuration. Any objections to closing? |
|
The effort has mostly already been spent, the removal date for legacy resolver is unknown, no-strict-optional is totally evil, so if it were me I would just merge. But feel free to close too. |
|
I'm OK with progressing this rather than closing it, but the first thing would be to address the review comments. |
|
Oh oops, pushed the change! |
pfmoore
approved these changes
Apr 20, 2024
uranusjr
approved these changes
Apr 23, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The assert in _set_req_to_reinstall is definitely correct, the three call sites have guards
The assert in _populate_link is a little trickier. It's not obvious to me how to prove it will never trigger. But do note that if link were ever None, it could potentially cause AttributeError in Cache._get_cache_path_parts and direct_url_from_link
The assert in _resolve_one is safe, if req_to_install.name was None it would raise TypeError in canonicalize_name
I can't prove that req.name is not None in get_installation_order and the code would work fine if that were the case (since it's a defaultdict), so I opted to just ignore