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
Add command line option to toggle parallel execution #349
Conversation
7dc7829
to
a1e989e
Compare
a1e989e
to
2125a4c
Compare
2125a4c
to
f317dea
Compare
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.
This change makes sense to me. Just a minor wording suggestion. 👍
Co-authored-by: Matt Clark <44023+mclark@users.noreply.github.com>
test/unit/packwerk/cli_test.rb
Outdated
@@ -296,5 +296,17 @@ def show_strict_mode_violations(_offenses) | |||
|
|||
cli.execute_command(["check", "--offenses-formatter=default", "--packages=components/platform"]) | |||
end | |||
|
|||
test "#parse_run (private) parses parallel option and overrides the configuration" do |
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.
Can we avoid calling the private method by calling the public interface and asserting the configuration object that we are already passing changed its value?
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.
@rafaelfranca
Oops. It certainly could have been achieved that way.
What do you think of these changes?
f318c59
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.
Thank you for writing tests against the public interface! That means I can refactor without needing to change tests! 🙇🏻♂️
#351 looks great work. |
Hello @meganemura. Thank you for your PR.
Thank you for worrying about this. It's okay, solving the merge conflicts was not too bad. It also allowed me to confirm that passing the I opened a PR to make small adjustments to your test, would you mind taking a look? 🙏🏻 |
What are you trying to accomplish?
Allows parallel configuration to be overridden by command line options.
This is useful if you want to switch parallel options between different execution environments, such as local and CI environments.
What approach did you choose and why?
I added the --parallel / --no-parallel option to the process of parsing command-line options by OptionParse.
What should reviewers focus on?
@parallel
in the Configuration class.That the test is against private methods.(fixed)Type of Change
Checklist