Check and fix non-inclusive laguange #303
Conversation
|
An initial thought when looking at the non-inclusive language check: the output from the test run isn't very 'useful'. Is that how the standard woke command reports warnings and errors? Since when looking at the CI run, I only see mentions insensitive words and its alternatives, but doesn't quite point to where the string appears in code. Would it be possible to make it pinpoint the location? I must say I never used Also, I think we'll have to wait with a final review of this card until Lukas and Steve are back as this basically requires changing the netplan schema, so we need to have an overall agreement on what we want it to be. |
slyon
added
the
schema change
There was a problem hiding this comment.
Thanks for setting up the new CI hook!
I think we should make use of # wokelint:rule=... comments throughout the code, to ignore exception (i.e. the things we cannot change).
Most cases are inside the tests, where we should be able to apply some search & replace logic to catch many cases at once. Maybe we need to adopt the assert_nm method in tests/generator/base.py to ignore # wakeignore: ... comments inside the provided keyfile.
For (internal) vairable naming, I'd not keep the old name around, but just rename the variable or struct fields accordingly, this should solve another class of warnings.
Integrate and make use of the project Woke to check for non-inclusive language in our codebase and documentation.
Replace some of the non-inclusive words caught by Woke.
- Add a rule to try to reduce the number of alerts for a term that,
unfortunately, is part of the networking jargon and can't be
replace without breaking backwards compatibility. The idea is to
add alternatives for it in the future.
- Add some terms from inclusivenaming.org that are not present in
the default ruleset.
Also, change the github workflow to fail on error.
Providing the alternative names below: all-slaves-active -> all-members-active packets-per-slave -> packets-per-member It keeps backwards compatibility by keeping the old code as it is and providing #defines for the new names. The yaml emitter will use the new and preferred terms by default.
It should include more details about the findings in the output.
Keep only the new one in the main title. Also, patch the validate_docs.sh script to also find the old term in its new place.
It's used by NetworkManager.
In the remaining files, the old terms are in the middle of raw strings that represent external configuration. It would be possible to workaround with string concatenation and insert the woke annotation there but it would complicate the code.
There was a problem hiding this comment.
Thank you very much for addressing the previous comments and adding a bunch of ignore-rules at the relevant places.
I left some more inline comments, which we could improve upon, while also waiting for @vorlonofportland 's opinion on the new all-members-active and packets-per-member stanzas (alias).
Generally, I wonder where we are supposed to get our woke ruleset from (and how to keep it updated), besides woke's default ruleset. I see you picked some additional rules from https://inclusivenaming.org and from https://github.com/canonical/Inclusive-naming How did you get to pick those? Is there any policy around this?
Another topic is about the blocklist naming, which I'm personally not a big fan of, as I tend to read "block" as a chunk of something (i.e. a "block of code"). I'd prefer the allow/deny terminology. But I see there's ongoing discussions on this topic also in canonical/apport#58
|
These schema changes look appropriate to me. The one thing I note is that the underlying backends (NM, networkd, and the kernel itself) still use the "slave" language. Will there be follow-up to try to promulgate this name change down the stack? |
ACK. I think it would make sense to cross-reference existing "inclusive-naming" bug reports in the corresponding backends, if already existing, or create such bug reports upstream. |
The wokeignore tag is also inserted in the middle of raw strings now. They are removed before passing the data to parsers. Remove the ignored files from the .woke.yaml file.
|
I created some upstream RFC's about using inclusive language in our backends networkd and NetworkManager:
The Open vSwitch did the change already in v2.15: Some recommendations from the Kernel itself: |
Description
This PR introduces a new github action used to check for non-inclusive wording in our code base and documentation using Woke.
While not all the occurrences of non-inclusive words were fixed, it also replaces many occurrences with proper language. Most of the remaining occurrences are due to external APIs and tools we rely on.
It provides alternative names for the options below:
all-slaves-active -> all-members-active
packets-per-slave -> packets-per-member
The yaml emitter will produce yaml documents using the new words.
Ideally, every single unit test containing the old words should be duplicated to also handle the new one, but at the moment only two tests were adapted.
Checklist
make checksuccessfully.make check-coverage).