Inclusive language#23283
Conversation
baptiste-b-pegasys
requested review from
fjl,
gballet,
holiman,
karalabe,
rjl493456442 and
zsfelfoldi
as code owners
|
you often change "like crazy" to unpredictably where it is still predictable. Think the meaning is often more like "with high frequency" |
|
Some of these changes cannot be made for backwards-compatibility reasons. |
|
Thank you for your PR. Our policy is to strive to use a more inclusive language in our new PRs, while not accepting modification with a large footprint, that do not bring a technical fix and clobber the git history. I shall also point out that "with a high frequency" does not correctly capture the meaning of "like crazy", as the former incorrectly conveys a meaning of regularity while the latter doesn't. The more correct concept would be "with high intensity". Also, "AllowList" is not grammatically correct: verbs in the infinitive can not function as adjectives. I suggest using the past participle "AllowedList". |
|
appart the grammar, what is the suggestion? To split it into small PRs? |
|
Yeah I think smaller PRs might be better. Some of them are internal renamings, other actually modify the API. |
And actually, I think neither "unpredictably" or "with high frequency" or "with high intensity" captures the meaning of "do X like crazy". |
|
I kind of agree with @gballet here.Clearly care should be taken to avoid potentially problematic phrasings, but I don't really see the point to go and change existing code just to replace wording where there was never any mal-intent. |
|
I reviewed it a bit more thoroughly. I think many of the changes were good, even better than the original. Like, "// Don't scan the USB too frequently if the user fetches wallets in a loop" is IMO better than "// Don't scan the USB like crazy it the user fetches wallets in a loop". I'm thinking that abiding by a set of rules makes things easy. |
holiman
left a comment
holiman
left a comment
There was a problem hiding this comment.
yeah I know it's closed, adding my 5c anyway
| WhitelistFlag = cli.StringFlag{ | ||
| DeprecatedAllowListFlag = cli.StringFlag{ | ||
| Name: "whitelist", | ||
| Usage: "[DEPRECATED: you shall replace it by 'allowlist'] Comma separated block number-to-hash mappings to enforce (<number>=<hash>)", |
There was a problem hiding this comment.
We actually have a system for deprecating flags, making them not appear in the normal listing, but in the extended help.
Also, "you shall" sounds a bit ... dramatic?
| parts := strings.Split(entry, "=") | ||
| if len(parts) != 2 { | ||
| Fatalf("Invalid whitelist entry: %s", entry) | ||
| Fatalf("Invalid allow list entry: %s", entry) |
There was a problem hiding this comment.
Seems like it should be one word, "allowlist"
| // | ||
| // If a newly added transaction is marked as local, its sending account will be | ||
| // whitelisted, preventing any associated transaction from being dropped out of the pool | ||
| // allowed, preventing any associated transaction from being dropped out of the pool |
There was a problem hiding this comment.
not "will be allowed", but "will be added to the allowlist"
| } | ||
| // Otherwise if it's a whitelisted block, validate against the set | ||
| if want, ok := h.whitelist[headers[0].Number.Uint64()]; ok { | ||
| // Otherwise if it's a allowed block, validate against the set |
There was a problem hiding this comment.
All blocks are allowed, but only some of them are in the allowList.
| } | ||
|
|
||
| // RegisterApisFromWhitelist checks the given modules' availability, generates a whitelist based on the allowed modules, | ||
| // RegisterApisFromAllowList checks the given modules' availability, generates a allow List based on the allowed modules, |
| } | ||
| if d.netRestrict != nil && !d.netRestrict.Contains(n.IP()) { | ||
| return errNotWhitelisted | ||
| return errNotAllowedListed |
There was a problem hiding this comment.
NotAllowedListed? That's a double tempus form. NotAllowlisted maybe
| const ttlScaling = 3 | ||
|
|
||
| // ttlLimit is the maximum timeout allowance to prevent reaching crazy numbers | ||
| // ttlLimit is the maximum timeout allowance to prevent reaching unexpected numbers |
There was a problem hiding this comment.
s/unexpected/out of bounds
6 participants
I propose some changes on terms to be more inclusive, following this guideline: https://www.aswf.io/blog/inclusive-language/
The biggest impact is the
whitelistflag that needs to be deprecated, then removed in a later version.There are a lot of terms in
signer/fourbyte, do you think it could be done in a later PR?