Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eslint): add inclusive-language rule #71

Merged
merged 4 commits into from
May 25, 2021

Conversation

smackfu
Copy link
Member

@smackfu smackfu commented Apr 23, 2021

Description

Adds the "inclusive-language/use-inclusive-words" rule:
https://github.com/muenzpraeger/eslint-plugin-inclusive-language

The list of words is explicit to reduce the chance of unintended changes from the upstream dependency.

The rule is currently marked as a warning. Not sure if an error would be warranted?

Please make sure that the PR fulfills these requirements

  • I checked that there aren't other open Pull Requests for the same update/change.
  • My code follows the code style of this project.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • Rule changes includes a comment to describe the reasoning behind the change.
  • PR contains a single rule change.

Motivation and Context

Using inclusive coding terms is essential to creating an inclusive environment. And anything that can be automatically tested by eslint should be.

How Has This Been Tested?

Ran npm pack and installed the repo against an existing repository that was using the existing lint config. After adding a violation to the code, running eslint generated the expected warning:

  10:3   warning  The usage of the non-inclusive word 'master' is discouraged, use 'main' instead  inclusive-language/use-inclusive-words

Also, you can see the suggestions in WebStorm here:

Screen Shot 2021-04-23 at 1 13 25 PM

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update

@smackfu smackfu requested a review from a team as a code owner April 23, 2021 17:14
Copy link
Contributor

@PixnBits PixnBits left a comment

Choose a reason for hiding this comment

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

The comments on unicorn.js point out the usage of "master" in URLs
Looking I found muenzpraeger/eslint-plugin-inclusive-language#1 (comment) which noted that "HEAD" could be used instead. IDK if all users have GitHub URLs but those are likely to be the bulk and others can add a eslint-disable-next-line if no other URL works

Might be good to update the URLs in unicorn.js in this PR

index.js Outdated Show resolved Hide resolved
@smackfu
Copy link
Member Author

smackfu commented May 4, 2021

Might be good to update the URLs in unicorn.js in this PR

I fixed them in #70 by linking to tags, which actually is more correct since some of the links went 404 in the newer releases.

@smackfu smackfu requested a review from PixnBits May 4, 2021 17:29
@nellyk
Copy link
Contributor

nellyk commented May 5, 2021

@smackfu Thank you so much for your contributions! since we are encouraging the use of inclusive language, i'm proposing we use error since resolving this would be most likely a text change and most code bases are changing this. If someone isn't aware they might ignore the warning and reuse them again

infoxicator
infoxicator previously approved these changes May 6, 2021
Copy link

@infoxicator infoxicator 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 going for error instead of warn is also fine, people tent to be more proactive when it stops what they are doing rather than a warning that could be missed

@smackfu
Copy link
Member Author

smackfu commented May 6, 2021

@infoxicator @nellyk I changed it to be an "error" instead of "warning". Thanks for the feedback!

@smackfu smackfu requested review from nellyk and a team May 6, 2021 19:35
@@ -1,3 +1,4 @@
/* eslint-disable inclusive-language/use-inclusive-words */
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this be disabled just for the rule configuration section (as I'm guessing that's where there are errors) but we also want to get this feature out

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point, I just figured all the lines in that section would need the exclusion which might make it less clear.

@marcusrbrown marcusrbrown merged commit 5a1cf81 into americanexpress:main May 25, 2021
oneamexbot added a commit that referenced this pull request May 25, 2021
# [14.2.0](v14.1.1...v14.2.0) (2021-05-25)

### Features

* **eslint:** add inclusive-language rule ([#71](#71)) ([5a1cf81](5a1cf81))
@oneamexbot
Copy link
Contributor

🎉 This PR is included in version 14.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

nellyk added a commit that referenced this pull request May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants