Skip to content
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

Auto disable parallel when combined with --auto-correct, --auto-gen-config, or --fail-fast #9647

Merged
merged 1 commit into from Apr 15, 2021

Conversation

rrosenblum
Copy link
Contributor

This came up in a conversation in #9640. We probably should have made it work this way from the start instead of raising an exception.

It felt a big weird to handle this inside of a "validation". Please let me know if there is a better place for the code or if there is better error message.

I left in the code to delete the parallel flag from the rake task to avoid printing the warning about parallel being ignored.

@@ -362,16 +362,19 @@ def validate_parallel

def validate_parallel_with_combo_option
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should rename this method now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought. I couldn't think of great name for what this is doing. I think this is the first time we're disabling incompatible configs so maybe a new method entry point? Maybe something like fix_invalid_parallel_combos or disable_invalid_parallel_option?

Copy link
Member

@dvandersluis dvandersluis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rrosenblum thanks! I was going to get to this but hadn't gotten a chance yet. This is a great implementation.

One note: maybe your changed tests should expect that there is no parallel key in options after parsing?

@rrosenblum
Copy link
Contributor Author

rrosenblum commented Mar 30, 2021

One note: maybe your changed tests should expect that there is no parallel key in options after parsing?

@dvandersluis I like that idea, any thoughts on how to implement the check? options doesn't appear to expose the underlying data structure for easy checking. Best I can think of is expect(options.instance_variable_get('@options').keys).not_to include(:parallel)

@dvandersluis
Copy link
Member

Sorry Ryan, I had intended to give you an answer to your question and totally forgot. We could conceivably add a delegate to keys or key? (if that exists) inside the Option class if you want, but if you don't want to change that going through the ivar seems to be the only way.

@rrosenblum
Copy link
Contributor Author

It's cool. Unless you have strong feeling about it, I'm inclined to use instance_variable_get in the tests for now. I rather not expose the internal structure of options just to help with this 1 test. If there are some other use cases, then it could be worth while.

@bbatsov bbatsov merged commit 26f3cc3 into rubocop:master Apr 15, 2021
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 15, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants