Skip to content

Add MultilineMethodCallIndentation cop#6786

Merged
n1zyy merged 2 commits intomainfrom
mattw/multiline_method_call_indented
Aug 18, 2022
Merged

Add MultilineMethodCallIndentation cop#6786
n1zyy merged 2 commits intomainfrom
mattw/multiline_method_call_indented

Conversation

@n1zyy
Copy link
Contributor

@n1zyy n1zyy commented Aug 18, 2022

Enforce 'indented' style, and correct existing occurrences

Why: Currently multi-line method calls do not have to be indented at all, and where they are, some use 'indented' and some use 'aligned'. Let's standardize on one for cleaner code.

changelog: Internal, Rubocop, Enable MultilineMethodCallIndentation=indented

Enforce 'indented' style, and correct existing occurrences

changelog: Internal, Rubocop, Enable MultilineMethodCallIndentation=indented
@n1zyy
Copy link
Contributor Author

n1zyy commented Aug 18, 2022

The bulk of these changes are moving from 'aligned' to 'indented', though note that we'd have had about as much if we chose 'aligned' instead.

There are a couple cases where we fix stuff like this:

      allow(IdentityConfig.store).to receive(:proofing_device_profiling_collecting_enabled).
      and_return(ff_enabled)

The fact that rubocop wasn't catching that is what led me down this rabbit hole in the first place.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I think I prefer this to the aligned option in #6787, though I could see how one might prefer the other. One reason I see in favor of this option is that it's much easier to indent one extra level in my editor than it is to try to align it to the operator on the previous line. Either way, I'm all for the consistency.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM! Agree this is better than the alternative. I have a few things I'd manually change but otherwise this is great!

Comment on lines +323 to +324
return prompt_to_setup_mfa if service_provider_mfa_policy.
user_needs_sp_auth_method_setup?
user_needs_sp_auth_method_setup?
Copy link
Contributor

Choose a reason for hiding this comment

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

this one is weird. I think we should use the block syntax in cases like this?

if service_provider_mfa_policy. user_needs_sp_auth_method_setup?
  return prompt_to_setup_mfa
end

Copy link
Contributor Author

@n1zyy n1zyy Aug 18, 2022

Choose a reason for hiding this comment

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

The problem I see here is that there's a lot of them. I'd probably convert the whole block, which is currently:

    return prompt_to_setup_mfa unless two_factor_enabled?
    return prompt_to_verify_mfa unless user_fully_authenticated?
    return prompt_to_setup_mfa if service_provider_mfa_policy.
      user_needs_sp_auth_method_setup?
    return prompt_to_setup_non_restricted_mfa if two_factor_kantara_enabled?
    return prompt_to_verify_sp_required_mfa if service_provider_mfa_policy.
      user_needs_sp_auth_method_verification?

Mixing them looks weird otherwise.

Incidentally, this one line doesn't actually have to be broken to not be too long, but the one a few lines down does.

Are you on board with me converting this whole section? It'll be a bit longer but I think it's for the best.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind mixing, but I do think that it would be preferable to have the blocks to the split trailing ones like originally proposed

Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

This is great, and I agree this is preferable to aligned!

return prompt_to_setup_non_restricted_mfa
elsif service_provider_mfa_policy.user_needs_sp_auth_method_verification?
return prompt_to_verify_sp_required_mfa
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachmargolis I'm on the fence here. I think this is better, but it doesn't look as good.

Also someone should double-check my logic since I just some unlesses into if !s and accidentally inverting logic here would probably be quite bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • I spot checked the unless -> if ! and that looks good
  • I think this if/else looks great

unless user_fully_authenticated? && service_provider_mfa_policy.
auth_method_confirms_to_sp_request?
return confirm_two_factor_authenticated(request_id)
end
Copy link
Contributor Author

@n1zyy n1zyy Aug 18, 2022

Choose a reason for hiding this comment

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

This is what rubocop made me do. I'm not sure I love this; the unless continuation is further indented than the block contents.

The good news is that the method looks less like a map of Oklahoma. (Which is surely a great state, just not the shape a block of code should take.)

@n1zyy n1zyy merged commit cfbbcf7 into main Aug 18, 2022
@n1zyy n1zyy deleted the mattw/multiline_method_call_indented branch August 18, 2022 19:55
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.

4 participants