-
Notifications
You must be signed in to change notification settings - Fork 41
Several small improvements to build-script-helper.py #94
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
Conversation
09ec338 to
4b72a26
Compare
The test action creates its own build and building twice just wastes resources.
Previously also generated SwiftEvolve Xcode projects was name SourceKitStressTester.
4b72a26 to
55f4381
Compare
Does build-script ask us to just test when it assumes that this will also cause a build? If so, this may not be the right move. |
|
|
CI testing passed: https://ci.swift.org/job/swift-PR-osx-preset/64/ |
nathawes
left a comment
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.
LGTM!
| def should_run_action(action_name, selected_actions): | ||
| if action_name in selected_actions: | ||
| return True | ||
| elif "all" in selected_actions: |
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.
Not important, but you could return "all" in selected_actions at this point, or replace the whole body with something like return any(item in (action_name, "all") for item in selected_actions).
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 actually find it clearer this way. The second suggestion means, I actually need to think about what’s going on and I like repeating the same pattern that I used for action_name already for "all". It’s a bit more verbose but I find it reads clearer.
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.
No worries!
This PR contains a set of small improvements to
build-script-helper.py, including the one from #91.