Skip to content

[FIx #10356] Add AllowConsecutiveConditionals option to Style/GuardClause#10503

Merged
bbatsov merged 1 commit intorubocop:masterfrom
ydah:add-option-guard-clause
Apr 9, 2022
Merged

[FIx #10356] Add AllowConsecutiveConditionals option to Style/GuardClause#10503
bbatsov merged 1 commit intorubocop:masterfrom
ydah:add-option-guard-clause

Conversation

@ydah
Copy link
Member

@ydah ydah commented Apr 4, 2022

Resolve #10356

This PR add AllowConsecutiveConditionals option to Style/GuardClause and the option is false by default.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@ydah ydah force-pushed the add-option-guard-clause branch 2 times, most recently from 8cc5a6e to deeb70c Compare April 4, 2022 17:03
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 6, 2022

I'm fine with the proposal in principle, but I don't like very much the name of this variable. Something like AllowMultipleSameDepthConditionals sounds a bit better, even if it's longer. Naming is hard, maybe @rubocop/rubocop-core have better ideas.

@ydah ydah force-pushed the add-option-guard-clause branch from deeb70c to b7e29c0 Compare April 6, 2022 15:48
@ydah ydah changed the title [FIx #10356] Add ExcludeSameLevelMultipleIf option to Style/GuardClause [FIx #10356] Add AllowMultipleSameDepthConditionals option to Style/GuardClause Apr 6, 2022
@ydah ydah changed the title [FIx #10356] Add AllowMultipleSameDepthConditionals option to Style/GuardClause [FIx #10356] Add AllowMultipleSameDepthConditionals option to Style/GuardClause Apr 6, 2022
@ydah
Copy link
Member Author

ydah commented Apr 6, 2022

My naming of variables was certainly not good. I think AllowMultipleSameDepthConditionals looks better.
I updated this PR.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 7, 2022

Btw, there's one detail of this we need to clarify - do we want to apply it if there's any conditional at the same depth or only if the preceding expression is a conditional at the same depth. The implementation seems to imply the former, but at least to me it seems that that latter probably makes more sense. The OP (@johnnyshields) is welcome to share his thoughts as well.

@johnnyshields
Copy link

@bbatsov thanks for pinging me. I'm somewhat agnostic to either approach. I think I might favor it to be the "only if the preceding expression is a conditional at the same depth", but probably either is fine.

@ydah ydah force-pushed the add-option-guard-clause branch from b7e29c0 to fa8f64d Compare April 8, 2022 03:24
@ydah
Copy link
Member Author

ydah commented Apr 8, 2022

@bbatsov @johnnyshields Thanks so much for the feedback!
Certainly, I'd like "only if the preceding expression is a conditional at the same depth" is better.

I updated this PR. Thank you so much!

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 8, 2022

Good. Now that we've settled this I have a proposal how to simplify the config name further - "AllowConsecutiveConditionals". That's essentially the same as before, but a bit more specific and easier to parse mentally.

@johnnyshields
Copy link

@bbatsov yep perfect name its 100% clear.

…le/GuardClause`

This PR add `AllowConsecutiveConditionals` option to
`Style/GuardClause` and the option is false by default.

I'd prefer to keep both if statements
if triggered offense at the same depth.

This option works only if the preceding expression
is a conditional at the same depth.

Since the existing behavior isn't erroneous,
this PR have made it optional and set default to false.
@ydah ydah force-pushed the add-option-guard-clause branch from fa8f64d to dd67ef4 Compare April 8, 2022 06:54
@ydah
Copy link
Member Author

ydah commented Apr 8, 2022

@bbatsov Yeah perfect name! I updated this PR. Thank you so much!

@ydah ydah changed the title [FIx #10356] Add AllowMultipleSameDepthConditionals option to Style/GuardClause [FIx #10356] Add AllowConsecutiveConditionals option to Style/GuardClause Apr 8, 2022
@ydah ydah changed the title [FIx #10356] Add AllowConsecutiveConditionals option to Style/GuardClause [FIx #10356] Add AllowConsecutiveConditionals option to Style/GuardClause Apr 8, 2022
@bbatsov bbatsov merged commit 4094375 into rubocop:master Apr 9, 2022
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 9, 2022

Thanks for tackling this! 🙇‍♂️

@ydah ydah deleted the add-option-guard-clause branch April 9, 2022 08:19
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.

Feature Request: Style/GuardClause should have an option to skip in cases where there are multiple if statements

3 participants