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

Add eslint-plugin-inclusive-language at error level #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

elifitch
Copy link

@elifitch elifitch commented Jul 9, 2020

This adds eslint-plugin-inclusive-language at the error level, so we're notified when we use uninclusive language such as whitelist or blacklist. Currently this plugin only offers a minimal, basic start, but we can update the package as it grows. https://github.com/muenzpraeger/eslint-plugin-inclusive-language/blob/primary/lib/config/inclusive-words.json.

I'm also targeting this PR at a new branch, main, that we can use in lieu of master, but I don't have permissions to change the base branch of this repo.

Unsure who should review, so tagging mine own team and devtools.

cc @mapbox/map-design-team @mapbox/developer-tools

@elifitch elifitch requested review from rclark and davidtheclark July 9, 2020 16:42
@davidtheclark
Copy link
Contributor

Rather than adding this to base.js, let's add support for the plugin in the same way that we do for other plugins: as separate configs with their own peer dependencies. In the past, at least, ESLint plugins could not be installed as transitive dependences, as you've done here: they've needed to be peer dependencies. (Has that changed?) See https://github.com/mapbox/eslint-config-mapbox#plugin-specific-configurations.

@elifitch
Copy link
Author

elifitch commented Jul 9, 2020

@davidtheclark Didn't know about the peer dependency thing, totally cool to make that change. My concern for treating this the way we do react, promise and import is I feel like we should make it the default to error if code includes racist language, rather than asking teams to have to bring in an additional config in order to get this benefit. Is there a way to accomplish that w/o adding this plugin to base.js?

@davidtheclark
Copy link
Contributor

davidtheclark commented Jul 9, 2020

Is there a way to accomplish that w/o adding this plugin to base.js?

I don't know of one. When I was last digging into this stuff, I thought it was a silly limitation of ESLint's plugin system that shared config packages could not install their own plugins — that the plugins needed to be peer dependencies. That was at least a couple of major versions ago, so I think it's worth checking whether this is in fact possible with ESLint v7.

You could use beta releases to try different dependency setups in some sample repos.

Seems we have two options:

  • Implement and document this like other plugin configs. In this case, repo owners would choose to incorporate it, see how it goes, turn off and on, etc. There's no enforcement that all package consumers also use it. Lots of consumers probably won't use it mostly because they don't know about it or don't think of it.
  • Or we could implement this in the base config (whether or not we can avoid a peer dependency). Then everyone who installs or updates the package will be compelled to use it right away, or else take an action to deliberately disable it.

What do you think? Do you feel good enough about how the plugin works that you think we should jump right to base.js, because it will enforce some simple and valuable constraints without undue friction?

Let's make sure to add documentation to the PR, either way. I also think it might be a good idea to stick with the current branch naming for this PR, then change it in a followup, instead of bundling the two? (I also do not have admin rights to this repo.)

@mbullington
Copy link

It seems like this is still in flux, but I found a (somewhat well used?) solution to the transient dependencies problem: https://github.com/microsoft/rushstack/tree/master/eslint/eslint-patch

I have no clue who the owner for this repo would be now, but I'd be comfortable adding a v4.0.0 with this patch and other plugins, making eslint-config-mapbox significantly easier to use (and more inclusive).

cc @tristen @adrianababakanian @kscoulter

@mbullington
Copy link

Also are any of you admins on this repo? We should change it to main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants