Enable and configure Naming/InclusiveLanguage #288
Conversation
rubocop.yml
Outdated
| CheckIdentifiers: true | ||
| CheckConstants: true | ||
| CheckVariables: true | ||
| # CheckStrings is false in the rubocop defaults | ||
| CheckStrings: true | ||
| CheckSymbols: true | ||
| CheckComments: true | ||
| CheckFilepaths: true |
There was a problem hiding this comment.
IMO this new cop (which helps us enforce the rule introduced by #167, by the way) makes sense for naming in variables, constants, etc, but when it comes to the contents of strings it doesn't make much sense to me. I think we should omit this configs and stick with RuboCop's defaults.
There was a problem hiding this comment.
My counterargument for including strings is that it catches references in error messages ("add xyz to whitelist to allow ...") and test descriptions. Test descriptions often have references to the same identifiers, constants, variable where we'd look for offenses.
There was a problem hiding this comment.
Hmm I'm not sure if these cases are strong enough to enable this check for strings. Having tests descriptions in sync is not the goal of this cop anyway. And when in doubt I'd say it's best to stick with RuboCop's defaults, assuming that having this setting as false was already a decision upstream.
There was a problem hiding this comment.
I won't belabor the point anymore after this, but if the goal of the cop is to make code bases more welcoming by eliminating problematic language then I'd still advocate for checking everywhere. The cop is in the Naming department because that was the best fit among the existing. I don't think it has to be limited by that.
As the cop author, I was not overly concerned with the defaults because I expected organizations to customize them for their individual needs. The upstream calibration may lean towards reducing noise for people when enabling new rules. I think we can be more opinionated here if we want to make progress on eliminating problematic terms.
Based on the feedback so far, I've removed the CheckStrings override from this change.
rubocop.yml
Outdated
| FlaggedTerms: | ||
| whitelist: | ||
| Regex: !ruby/regexp '/white[-_\s]?list/' | ||
| Suggestions: | ||
| - allowlist | ||
| - permit | ||
| blacklist: | ||
| Regex: !ruby/regexp '/black[-_\s]?list/' | ||
| Suggestions: | ||
| - denylist | ||
| - block | ||
| master: | ||
| Suggestions: | ||
| - primary | ||
| - leader | ||
| AllowedRegex: | ||
| - !ruby/regexp '/master[_\s\.]key/' # Rails master key | ||
| - 'blob/master/' | ||
| - 'origin/master' | ||
| slave: | ||
| Suggestions: | ||
| - replica | ||
| - secondary | ||
| - follower |
There was a problem hiding this comment.
Why aren't these the defaults from RuboCop itself?
There was a problem hiding this comment.
The Rubocop defaults dropped master from the configuration after this was initially released: https://github.com/rubocop/rubocop/blob/master/config/default.yml#L2537-L2560
Checking for master was viewed as generating too noise from this cop.
Other than that, these are the Rubocop defaults except for changing the format of Suggestions to be consistent.
After looking at some grok searches we may also want to add an allowed use for web_console.whitelisted_ips.
There was a problem hiding this comment.
Got it. I think we should only define the configs we are overriding. If you dump the full config we can assert that the merged result is the expected one during the review (and also make the PR pass CI).
There was a problem hiding this comment.
That's a good point on the full config dump catching changes. This is my first time contributing to this repo, so I hadn't encountered that before.
I've updated the config here to just include overrides and included the full config dump. This has a couple of other updates from the Rubocop version change.
|
Shopify's style guide prefers avoiding terms like "white-list" and "black-list". Drop in replacements include "allow-list" and "deny-list", or "clearlist" and "blocklist". Also consider if there is a domain specific term you could use. For example, "parameter white-list" could be replaced with "parameter allow-list", but "trusted parameters" may more suitable. |
rubocop.yml
Outdated
| FlaggedTerms: | ||
| whitelist: | ||
| AllowedRegex: | ||
| - 'web_console.whitelisted_ips' |
There was a problem hiding this comment.
Not sure if it's worth setting this regex for all users of the gem. This is not a config that comes by default from Rails (or web console), and I would expect users that need to define a whitelist of IPs to whitelist this regex by hand in their RuboCop config. This is different than the master key file, that comes by default, therefore we need to allow it in here.
There was a problem hiding this comment.
Happy to remove it, but this was included in Shopify Rails apps generated prior to March: https://github.com/Shopify/shopify-rails/pull/399
The configuration was deprecated in web-console in v4.0.3, but is currently still supported.
| AllowedRegex: | ||
| - 'web_console.whitelisted_ips' | ||
| master: | ||
| Suggestions: |
There was a problem hiding this comment.
I think main is nowadays the preferred suggestion for master. Could you please include it in the list?
There was a problem hiding this comment.
Added. Context definitely matters here but that is difficult for a cop to determine. main makes sense for a branch, but primary or leader make sense for a node or server.
tjwp
force-pushed
the
tjwp-inclusive-language
branch
from
e66d106
to
577a61e
Compare
tjwp
force-pushed
the
tjwp-inclusive-language
branch
from
577a61e
to
293b13a
Compare
This is a follow-up to https://github.com/Shopify/shopify-cops/issues/204.
A generic Naming/InclusiveLanguage cop has been included with the recent
v1.18.xrelease of Rubocop.The cop can be configured to search in various places for problematic terms and suggest more inclusive language.
This pull request proposes that we enable the cop and explicitly set all configuration. I don't recommend relying on defaults for this cop because I expect they may continue to change in rubocop. A day after the initial release
masterwas removed from the flagged terms, andCheckStringswas flipped to false.I'm hoping that this pull request can be used to discuss the specifics of the configuration that we want to enable, especially with respect to the flagged terms.
Note: I'll update the config dump once there is agreement on the configuration.