Enable Rubocop predicate naming check and fix a few#10990
Conversation
changelog: Internal, Source code, Add and fix predicate naming lint
n1zyy
left a comment
n1zyy
left a comment
There was a problem hiding this comment.
I think I agree that several of these are made more clear with your change.
I don't think I would vote to enforce this via rubocop, but I also won't protest if others decide we should.
| private | ||
|
|
||
| def is_pending_and_established_between(early_benchmark, late_benchmark) | ||
| def pending_and_established_between(early_benchmark, late_benchmark) |
There was a problem hiding this comment.
This change makes a lot more sense to me and is more in keeping with the spirit. The "is" for something that's not a boolean is confusing; this makes it much clearer. 👍
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
- Old method name: user_has_gov_or_mil_email?
| :user_agent | ||
|
|
||
| delegate :two_factor_enabled?, to: :mfa_policy | ||
| delegate :has_gov_or_mil_email?, to: :user, prefix: :user |
There was a problem hiding this comment.
looks like this was complete unused
I think it's important that if we have a rule, it be enforced in code as much as possible! Then we don't have to have unreliable humans opining. Since |
|
I lean towards agreement with @n1zyy. I agree that if there's a rule, it should be enforced automatically if possible, but I don't know if this is something that should be automatically enforced. |
I think the PR in its current state is a good middle ground/compromise state so I think I'm ready to merge it? We can always revert if it causes a lot of headaches or eye rolls |
changelog: Internal, Source code, Add and fix predicate naming lint * Add back prefix for has_gov_or_mil_email? * Allow has_ prefix for predicate names Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com> * Clean up other request method name * Remove unused delegator on TwoFactorOptionsPresenter - Old method name: user_has_gov_or_mil_email? * Bring back a has_ --------- Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
4 participants
One style guide suggestion for Ruby is to avoid prefixing predicate methods, and Rubocop has an option to enforce this
I tried enabling it, and I like most of the results. The
SimpleFormgem has ahas_errors?method so I was a little hesitant to rename some of our own usages of it.However, I left a few since I think they might read better as is, open to feedback on what others think. These are the ones I didn't rename that will fail the build:
has_errors?has_hint?has_remember_device_auth_event?has_other_auth_methods?has_minimum_required_mfa_methods?has_in_person_enrollment?has_establishing_in_person_enrollment_safe?has_pii_from_user_in_flow_session?