-
Notifications
You must be signed in to change notification settings - Fork 760
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
Refactor project interpreter request for requires-python
specifiers
#4216
Conversation
fc7e190
to
1321ccf
Compare
2c1499b
to
8e63d1d
Compare
1321ccf
to
d19f861
Compare
1a449ca
to
662a4c2
Compare
8e63d1d
to
f444f3c
Compare
requires-python
specifiersrequires-python
specifiers
) | ||
} else { | ||
Toolchain::find( | ||
None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess I might want to make find
take a Option<ToolchainRequest>
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally find it a little easier to read as:
let interpreter = python
.map(ToolchainRequest::parse)
.or(requires_python
.map(|specifiers| ToolchainRequest::Version(VersionRequest::Range(specifiers.clone()))))
.map_or_else(
|| {
Toolchain::find(
None,
// Otherwise we'll try to use the venv we just deleted.
SystemPython::Required,
preview,
cache,
)
},
|request| {
Toolchain::find_requested(
&request,
// Otherwise we'll try to use the venv we just deleted.
SystemPython::Required,
preview,
cache,
)
},
)?
.into_interpreter();
But totally subjective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh that's really nice haha but... I think I still want to refactor Toolchain::find
to just not take strings anymore so I'll probably just merge and change it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want it in the release and it's after work hours! hehe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do really like it actually <3
Refactor following #4214 to avoid parsing the specifiers again