-
Notifications
You must be signed in to change notification settings - Fork 276
chore: pattern matching #2434
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
chore: pattern matching #2434
Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Do you mind if we punt this to after v3.0? Big diffs like this seem like unecessary risk when tapering to a release. |
Sure, I even almost added the tag when I opened it. |
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.
Pull Request Overview
This PR converts several conditional chains to Python's pattern matching syntax to streamline the code.
- Refactored AST checking in projectfiles.py
- Replaced if/elif chains with match-case in platforms modules (windows, macos, linux, ios) and oci_container.py
- Applied pattern matching for merging rules and settings conversion in options.py
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
cibuildwheel/projectfiles.py | Converted AST conditionals to a match-case construct for evaluating if a node is the main check. |
cibuildwheel/platforms/windows.py | Replaced string comparisons with match-case on build frontend values. |
cibuildwheel/platforms/macos.py | Updated build and setup routines to use pattern matching consistently. |
cibuildwheel/platforms/linux.py | Simplified build_in_container logic with match-case on build frontend names. |
cibuildwheel/platforms/ios.py | Enhanced build and setup logic by using pattern matching for cleaner control flow. |
cibuildwheel/options.py | Switched to match-case for merging options and stringifying settings, although a potential issue exists. |
cibuildwheel/oci_container.py | Replaced if-elif comparisons with a match-case structure for engine version validation. |
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.
The AST one is really nice !
I don't see a reason to hold-off merging that one but since it's already approved by @joerick and not merged yet so I won't merge.
yeah, let's get this in now. I'll let you hit the button henryiii, just in case there's something else you wanted to address here. |
Adding some pattern matching where I think it makes sense to do so.