-
Notifications
You must be signed in to change notification settings - Fork 232
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
feat: Windows filtering and sets #507
Conversation
57c963d
to
9128d8c
Compare
ad76134
to
550612d
Compare
Co-authored-by: Matthieu Darbois <mayeut@users.noreply.github.com>
550612d
to
a99e0de
Compare
Any opinions? This unifies and simplifies the handling of architectures a bit, but shouldn't be controversial? Unless we want docs changes? |
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.
Other than the one note on unsupported archs, this looks good.
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.
Oops, sorry, I thought I had already seen and reviewed this!
a99e0de
to
c97d3bc
Compare
This will change when #507 is merged
c97d3bc
to
fce7c70
Compare
Pulled arch checking into one place, and now they all behave the same way - specifying an empty set or an arch that's not allowed is an error. |
That's great. Already approved, but ... yep, good to merge, if you ask me! |
I didn't order the includes correctly in one place, but didn't want to rebuild after pushing so added #518 instead. I can fix it though. |
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.
Looks good!
|
||
import certifi | ||
|
||
from .environment import ParsedEnvironment | ||
from .typing import PathOrStr | ||
|
||
if sys.version_info < (3, 8): | ||
from typing_extensions import Literal |
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.
Ah, that's handy
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.
Everything that goes into typing that can be backported goes here. :)
Followup to #482, use sets instead of lists for Archs, and make Windows handling behave more like Linux, where one arch will only use that arch. (Eventually, maybe could include ARM for windows, perhaps?).
No docs changes (yet).
I converted to sets, then added the changes from @mayeut.