feat: add linting rules in JS and Ruby to encourage inclusive language #586
Conversation
| @@ -13,7 +13,7 @@ import { Notices, Notice } from './Notices' | |||
| const findByName = (items, item) => items.find(i => i.name === item.name) | |||
| const findById = (items, item) => items.find(i => i.id === item.id) | |||
|
|
|||
| const commonGenesToIgnore = ['Art', 'Career Stage Gene'] | |||
| const commonGenesBlacklist = ['Art', 'Career Stage Gene'] | |||
There was a problem hiding this comment.
As configured, this would result in the following error from ESLint:
app/models/artsy_tags_updater.rb
Outdated
| @tags_whitelist = tags_whitelist | ||
| @tags_blacklist = tags_blacklist |
There was a problem hiding this comment.
As configured, this would result in the following error from Rubocop:
There was a problem hiding this comment.
@anandaroop I just remembered this might not be sufficient. What is also happening is that our dependencies sometimes have not been updated. Is there concise language we could add here? From the RFC on inclusive language I suggest, and engineering directors/EVP agreed, that we should feel comfortable either addressing the issues ourselves on the dependency, opening pull requests, or submitting issues, depending on the size/culture of the third party system. If it's a dependency we are using that has the non-inclusive language, can we suppress the lint warning, do we live with it? etc.
There was a problem hiding this comment.
In special scenarios (when there is a reason that makes that specific part of the code too hard to be changed), I would say we handle this as we would handle any other issue pointed out by the linter. We could disable the rule only at that point so it won't block our work (in parallel, we can open PRs for the dependency, submit issues, etc).
There was a problem hiding this comment.
But I think we should update the linter language here to point out updating a dependency.
There was a problem hiding this comment.
I agree it would be nice to update the dependency whenever possible, but I'm unsure if we should have this as a goal for every situation.
During the recent efforts to update some dependencies, we noticed some scenarios where we're using some versions that are way outdated. In this case, even if we submitted a PR to the dependency repo with the inclusive language changes, I don't know if we would have enough capacity to perform the updates at our side from a very outdated dependency to the most recent version containing our changes (even though that would be ideal)
There was a problem hiding this comment.
I know - this was the problem discussed at the management level. It's not so much us as our dependencies, and the time needed to address them. However, IMO leaving very outdated dependencies is its own business risk. The harder it gets to update, the more serious we should take updating. (Tragedy of the commons.)
yarn add --dev eslint-plugin-inclusive-language
Our RFC suggests 'denylist' as an alternative to 'blacklist', so we add that along with the built-in suggestion of 'blocklist'
This commit is included just for demonstration purposes. This is the kind of change that would get flagged by the linter, and is only here because it was opted out of linting with --no-verify
This reverts the previous commit, which skipped linting via --no-verify for demonstration purposes only
…e rule - bump rubocop to >=1.21 - bundle update --minor regexp_parser rubocop (for minimal deps update that will satisfy this rubocop bump)
- accept autocorrections for new rules that are enabled by default - enable *all* new rules by default - accept autocorrections for (or selectively ignore) new rules
This commit is included just for demonstration purposes. This is the kind of change that would get flagged by the linter, and is only here because it was opted out of linting with --no-verify
This reverts the previous commit, which skipped linting via --no-verify for demonstration purposes only
anandaroop
force-pushed
the
roop/inclusive-linters
branch
from
8c87cb5
to
f89a7f8
Compare
anandaroop
changed the title
| @@ -81,7 +81,7 @@ | |||
| # config.logger = ActiveSupport::TaggedLogging.new(Syslog::Logger.new 'app-name') | |||
|
|
|||
| if ENV['RAILS_LOG_TO_STDOUT'].present? | |||
| logger = ActiveSupport::Logger.new(STDOUT) | |||
| logger = ActiveSupport::Logger.new($stdout) | |||
There was a problem hiding this comment.
Is this here for a dependency update? This doesn't on a glance seem to do with the task at hand. I might prefer in such a case just a quick note about it?
There was a problem hiding this comment.
That's right — this is one of several changes that was made in order to placate Rubocop in 0521c3b
And that's a consequence of upgrading this project to a recent enough Rubocop to get the new settings.
There was a problem hiding this comment.
Everything looks more or less right to me, but I might like @lidimayra to take a look since she suggested. I have one comment that stood out but I don't think it's a blocker. Thanks for the template @anandaroop.
|
Merging now, so I can start with the branch rename… |
In response to the RFC artsy/README#427 I wanted to see how easy it would be add an automation to encourage this cultural norm.
This PR introduces:
For Javascript — The
inclusive-languageplugin for ESLintFor Ruby — The
Naming/InclusiveLanguagerule for Rubocop👉🏽 Browse through the commits to see what's involved in installing and configuring.
For a project that already uses ESLint, it seems dead simple to install the necessary plugin.
For a project using Rubocop, one does have to upgrade to a recent enough version — ≥1.21 — to get the latest rule. This can be tricky depending on what kind of dependency spaghetti you have. In this case it involved just updating one other lib simultaneously. There also may be a little work involved in enabling/silencing/placating the most recent cops that come along with the upgrade.