Closed kathytafel closed 2 years ago
lidimayra
commented
2 years ago I completely agree. I also wonder if the RFC resolution steps could include having our teams upgrade rubocop in the owned projects.
Rubocop includes the InclusiveLanguage rule (link) since v1.23.0.
And having our linter updated sounds healthy to our codebase in many other aspects as well.
kathytafel
commented
2 years ago @lidimayra I added the note for linter. Is that sufficient, since linters will be different per repository/language?
lidimayra
commented
2 years ago Yes, I think so.
I mentioned rubocop, because this is the linter that we already use in our ruby-based projects and it would be a matter of updating what we already have. I don't know about the options for other languages, but it would surely be a nice opportunity to find that out for each repo.
anandaroop
commented
2 years ago Thanks, good to have this made explicit.
I think many of us have implicitly followed this since those Slack conversations (the precipitating example predates that), but there's also been a lot of new faces since then, and having an RFC to point to makes this clearer and more actionable.
Glad to learn about that new Rubocop setting (I hesitated to call it a "cop" because, well, you know 😅)
Seems that ESLint also has a similar plugin.
I think I'd vote to have these nudges via linting than via a checklist because:
Given that our two most common languages have the option to automate this, might we consider that instead of the checkbox?
lidimayra
commented
2 years ago Glad to learn about that new Rubocop setting (I hesitated to call it a "cop" because, well, you know 😅)
very good point, I also edited my message to remove this from the original sentence, thanks for raising it 🙌
And I'm also in favor of setting the linters over the checkboxes option
kathytafel
commented
2 years ago @anandaroop exactly my thought on the longevity of the conversation. Most everyone in the Berlin office was hired after the conversation, for instance, and also might not have as much context for what is more an American problem, so it's good to educated every once in a while.
What about a short-lived checkbox until the baselines per repository are done. That is, main --> master migrated and linter updated with these language rules? My thought had been to not make extra work out of the normal product time. Outside of a checkbox, how would you get people to add the linter rules (also for Eigen!) and the repository name migration? We've already made it so new repositories will be called main. I agree that once those tasks are done the rest can be left to linting.
mzikherman
commented
2 years ago I'm in favor of this but I would disagree with the need for any checkbox or pull request template.
What about a short-lived checkbox until the baselines per repository are done. That is, main --> master migrated and linter updated with these language rules? My thought had been to not make extra work out of the normal product time.
If we want to switch main -> master, we need to afford time to do that or live with the time it will take to do that (as people/teams find appropriate, 'maintenance mondays', 'future fridays', regularly scheduled product work as part of 20% bug fixes, motivated parties championing or working on it it, etc.). I'm not sure why a checkbox on every PR would need to happen in the meantime. Same goes for any updated linter rules. Strongly in favor of automation to help identify or flag problematic language, but not necessarily in favor of PR template checkboxes otherwise.
anandaroop
commented
2 years ago Yeah I'm also not confident that a checkbox would have the intended effect, without a lot more prescriptiveness about the who/when/what of the action that it's supposed to induce 🤷🏽
But I do feel like the linting approach is super-clear. Who = you when you get the message. When = when you commit. What = whatever the linter is noodging you about.
I thought it could be helpful to see an example of the work involved in adding that linting to a project. So I used Rosalind as a guinea pig and opened https://github.com/artsy/rosalind/pull/586 to demonstrate this approach for both Ruby and Javascript
My takeaways:
kathytafel
commented
2 years ago I've updated to remove the PR checkbox. Agree that it just needs time to be done. Engineering management suggests we use this Future Friday to address.
pvinis
commented
2 years ago kinda off topic, but there is this on rubocop, https://github.com/rubocop/rubocop/issues/8091 and a blog post https://metaredux.com/posts/2020/06/08/the-rubocop-name-drama-redux.html.
kathytafel
commented
2 years ago We decided to do it.
3: Majority acceptance, with conflicting feedback only about how to get things done.
We implemented 85% of it. Will monitor in 6 months to see what's left
Eidolon and Energy have extra things to fix in order to deploy.
lidimayra
commented
2 years ago Adding a note regarding something that happened today and made me think about this RFC again.
In many scenarios during the FF mob, we manually replaced whitelist/blacklist terms by doing a "search & replace". This method prevents us from finding possible inflections of these terms.
As an example, today the RuboCop InclusiveLanguage rule was enabled on volt. By doing this, a scenario where the manual searching wouldn't suffice our needs was detected: usage of terms in the past tense (whitelisted).
I would be happy to work on subsequent FFs to attack this part of the issue.
Proposal:
"RFC: Rename 'master' branches to 'main' #386" was opened in April and speaks to 1/2 the proposal. We have not yet completed the job. This RFC is to get us over the finish line, with both the master/main topic as well as allow/deny lists. It is a small thing to change language, but with profound impact. Let's not perpetuate the master/slave or 'black = bad' tech metaphors.
Reasoning
All the reasoning in #386.
Exceptions:
Also the exceptions in #386. But no need to update archived repositories.
Additional Context:
As a new joiner, I was surprised when I saw a PR with 'blacklist' because I would have assumed that Artsy had the discussion when that happened in the industry. When asking about it, I was gratified to find out that my assumption was true, by being pointed to this thread from a former employee from 2 years ago referencing this post).
However, we still we have some dangling threads as evidenced by there still being outdated language in our repository. This RFC hopes to introduce some behaviour changes that will make it so we don't have the same conversation in a year.
Master --> Main In investigating the problem, engineering directors noticed that maybe people didn't feel comfortable renaming repositories, so this pull request updates the playbook for that.
Allow/Deny We also noticed that while we are migrating to allow/deny, some of our included dependencies use outdated language. We should not let it stop us that the outdated language comes from outside our organisation if we want to lead with openness, and follow through on myriad commitments to diversity as claimed here. To that end we should make sure in a pull request to notice use of calling a "blacklist/whitelist." If it's inside our org's repositories, we should either fix within that PR or open an issue if it is large surgery. If it's outside our organization, we should update the dependency if the other org has already modernised. If the org has not yet deprecated outdated language, we should either open a pull request or file an issue depending on the size of the problem.
How is this RFC resolved?
Agree to spend time updating repositories, dependencies, and linters. Engineering management suggests mobbing on it this Friday December 10. To follow along:
Reminder set on this RFC to check in 6 months for progress by searching our org for outdated language.