muenzpraeger / eslint-plugin-inclusive-language

An ESLint plugin to raise awareness for using inclusive language not only in your codebase, but in life.
MIT License
57 stars 10 forks source link

Allow references to GitHub urls #1

Closed merrywhether closed 4 years ago

merrywhether commented 4 years ago

Thanks for creating this! I looked on npm on a lark before trying to make my own and was happy to find this.

In documentation/explanatory comments, it is common to includes urls to code in Github and master is still the default branch for the majority of repos:

// Library doesn't handle this situation correctly
// https://github.com/.../blob/master/...

It would be awesome to allow this specific usage of the word master (perhaps disallow master except when preceded by /blob/ à la (?<!\/blob\/)master) since otherwise it requires a mid-comment eslint-disable... magic comment or could disincentivize contributors from including such links in the first place.

I agree with the push to change the default branch for git projects, but it's not necessarily feasible to update all of own's dependencies.

muenzpraeger commented 4 years ago

Good point. For these cases you can (and should) use the standard ESLint feature to disable validation for the affected lines.

merrywhether commented 4 years ago

Yeah, that's what I've done so far:

// eslint-disable-next-line inclusive-language/use-inclusive-words

And it's not a big deal, but would probably ease/increase adoption.

sarayourfriend commented 4 years ago

@muenzpraeger could this be addressed by https://github.com/muenzpraeger/eslint-plugin-inclusive-language/issues/6? If you allowed blob/master or origin/master and set the partial flag to true it would address this without adding eslint-disable all over the place.

muenzpraeger commented 4 years ago

@merrywhether Just published v1.2.0, that also includes #8 - LMK here if that solves your case.

muenzpraeger commented 4 years ago

Closing this for now, as it can be changed with #6 or // eslint-disable.

merrywhether commented 4 years ago

Sorry for being slow to respond to this, but investigated today and found that the urls still don't work due to the regex construction here:

regex = new RegExp(`[\\w-_]*${wordDeclaration.word}[\\w-_]*`, 'ig');

Since \w-_ doesn't include / a url like blob/master only passes master to the excludeAllowedTerms function.

sarayourfriend commented 4 years ago

@merrywhether Ah, I wish I'd actually paid attention to the use case when I wrote that regex 😞 FWIW, you can do what I did in this PR https://github.com/Automattic/wp-calypso/pull/43993 and replace master with HEAD in most GitHub URLs and remove the problematic language altogether (also solves the issue where projects you reference could potentially rename their master branches as HEAD just references the default branch's HEAD).

As for a solution built into the plugin, could a new configuration be added to allowedTerms objects that would use a separate regex? Or is it safe to just add / alongside -_?

muenzpraeger commented 4 years ago

I just published v1.2.1 with #14, so this should be fixed now.

merrywhether commented 4 years ago

Works perfectly, thanks!