-
-
Notifications
You must be signed in to change notification settings - Fork 930
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
fix(locale): filter inappropriate words in the en locale #1633
Conversation
IMO the English locale is big enough that we can be quite thoroughly with the removals. |
Team decision We will check:
|
8301ae2
to
2779c33
Compare
This PR is ready for review now. I do realize I probably went a little overboard on some of the words, so it's unlikely I'll have any arguments about words to reinstate. (With such long lists, I sort of had to turn part of my brain off and go by a gut feeling.) I did initially try a using a list of bad words meant for moderation software, but felt they lacked nuance. |
Please run |
2779c33
to
03d2a2f
Compare
03d2a2f
to
2266db7
Compare
Rebased on latest 'next' branch. Updated test snapshots. Squashed the commits into a single commit. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## next #1633 +/- ##
========================================
Coverage 99.64% 99.65%
========================================
Files 2240 2240
Lines 240291 240106 -185
Branches 1072 1076 +4
========================================
- Hits 239432 239271 -161
+ Misses 838 814 -24
Partials 21 21
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is a merge conflict with test/__snapshots__/system.spec.ts.snap
Please rebase your PR and solve it
I would like to softly "enforce" a required approval from @xDivisionByZerox for this PR
These merge conflicts are going to keep cropping up as other changes are merged in. I'm going to wait until this ticket is otherwise ready to be merged and handle conflicts then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I added some ":thinking:" to the first two files
I think we need to add a sentence / explanation to a file why some words are not allowed
Maybe the contrib guidelines, maybe a comment at the top of the file?
I'm not sure about the colors, we could say: no colors => use colors module for that and I'm fine
I'm not sure about some words like "lame" as yeah they might be like "boring" but I don't think they need to be totally omitted
"obese", "heterosexual", "homosexual" => I think it is more inclusive to have these words in our set as the otherway around
There are some alcoholic words 🤔 maybe we need to transfer these to the requested food module (like colors)
"dwarf" => it is just a fantasy word 🤔
I understand why to remove sexual affected words, but not sure where to draw the line between normal talk and "above-18" words
Line 5 in db6b555
Since there is no need to include any words regarding
we should avoid adding any of them, because we still have plenty of other words.
Maybe, I just don't know how to exactly formulate that.
This is somewhat special to the word category in general and not a single file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 We appreciate the cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also discussed to remove entries from locales where a file contains more then 1k entries (like names or nouns) so files are not getting bigger and bigger
1k entries are more then enough for normal use cases
67f96e7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve this for two reasons, even tho I'm not happy with some words being removed:
- It's reasonable to have this in the alpha release, as people are actually interested in this
- As @ST-DDT said: We have so many words in the
en
locale, it doesn't matter if we get rid of some
Addressing #1631