Skip to content

Add check for non-inclusive language - #54

Merged
t184256 merged 2 commits into
linux-system-roles:masterfrom
nhosoi:woke-action
Dec 20, 2022
Merged

Add check for non-inclusive language#54
t184256 merged 2 commits into
linux-system-roles:masterfrom
nhosoi:woke-action

Conversation

@nhosoi

@nhosoi nhosoi commented Dec 14, 2022

Copy link
Copy Markdown
Contributor

Add a check for usage of terms and language that is considered non-inclusive. We are using the woke tool for this with a wordlist that can be found at https://github.com/linux-system-roles/tox-lsr/blob/main/src/tox_lsr/config_files/woke.yml

CHANGELOG.md - cleanup non-inclusive words.

@Jakuje

Jakuje commented Dec 14, 2022

Copy link
Copy Markdown
Collaborator

Would be great if the action (or at least the commit message or PR description) would have some human-readable description what does the CI action does and why.

Comment thread .github/workflows/woke.yml Outdated
@@ -0,0 +1,18 @@
# yamllint disable rule:line-length
# name: woke

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# name: woke
name: woke

@richm

richm commented Dec 14, 2022

Copy link
Copy Markdown
Contributor

Would be great if the action (or at least the commit message or PR description) would have some human-readable description what does the CI action does and why.

Sorry - we get so caught up in the tasks that the core team is focused on, we forget that other people might not realize what we're doing.

@nhosoi We should update all of the existing PRs like this:

PR title: Add check for non-inclusive language

PR body: Add a check for usage of terms and language that is considered non-inclusive. We are using the woke tool for this with a wordlist that can be found at https://github.com/linux-system-roles/tox-lsr/blob/main/src/tox_lsr/config_files/woke.yml

in .github/workflows/woke.yml at the top-level it should have
name: Check for non-inclusive language

Then the PR status will look like Check for non-inclusive language / woke (pull_request)

@Jakuje is this sufficient?

Add a check for usage of terms and language that is considered
non-inclusive. We are using the woke tool for this with a wordlist
that can be found at
https://github.com/linux-system-roles/tox-lsr/blob/main/src/tox_lsr/config_files/woke.yml
@nhosoi

nhosoi commented Dec 14, 2022

Copy link
Copy Markdown
Contributor Author

Thank you for your suggestions, @richm. I think I updated the PR following your advice.

Sorry, @Jakuje, for giving you this surprise... Does this updated PR look better?

@nhosoi nhosoi changed the title Add github action woke.yml (github.com/get-woke/woke-action) [WIP] Add github action woke.yml (github.com/get-woke/woke-action) Dec 14, 2022
@nhosoi

nhosoi commented Dec 14, 2022

Copy link
Copy Markdown
Contributor Author

@richm, I put [WIP] to the PR title because once get-woke/woke#252 (or its alternative) is merged, we may need to add a new option to the action to allow the warning-level words. Do you think it's ok to wait for the woke PR's fate? Or should we handle the woke PR separately?

@nhosoi nhosoi changed the title [WIP] Add github action woke.yml (github.com/get-woke/woke-action) [WIP] Add check for non-inclusive language Dec 14, 2022
@Jakuje

Jakuje commented Dec 14, 2022

Copy link
Copy Markdown
Collaborator

Thanks for the improvements. Looks better. I have only concern about the modification of the changelog how it is done now.

@richm

richm commented Dec 14, 2022

Copy link
Copy Markdown
Contributor

Thanks for the improvements. Looks better. I have only concern about the modification of the changelog how it is done now.

Not sure what you mean - what's your concern about the changelog?

@richm

richm commented Dec 14, 2022

Copy link
Copy Markdown
Contributor

Thanks for the improvements. Looks better. I have only concern about the modification of the changelog how it is done now.

Not sure what you mean - what's your concern about the changelog?

Is it that the intention of the changelog is that it is readable by end users, and having lines like

- update to tox-lsr 2.4.0 - add support for ansible-test sanity with docker // wokeignore:rule=sanity

is going to be distracting?
We could tell woke to skip CHANGELOG.md completely - but then we run the risk that some sort of non-inclusive language getting into CHANGELOG.md, which would be even worse than having it in other code which isn't usually intended to be read.
We could edit CHANGELOG.md to change or remove those instances of non-inclusive language - this may be the best option.

@Jakuje

Jakuje commented Dec 15, 2022

Copy link
Copy Markdown
Collaborator

yes, the linter hint might be distracting or confusing to users. I think editing the changelog not to contain potentially non-inclusive language should be preferred way (this is why we do it anyway, aren't we).

@nhosoi nhosoi changed the title [WIP] Add check for non-inclusive language Add check for non-inclusive language Dec 16, 2022
@nhosoi

nhosoi commented Dec 16, 2022

Copy link
Copy Markdown
Contributor Author

[citest]

@nhosoi

nhosoi commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

[citest bad]

@nhosoi

nhosoi commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

@richm, @Jakuje, @spetrosi, could you please review this pr one more time?
(please note that I should not have to run citest since the changes added in this pr do not affect the citests at all... so, please ignore the failures.)
Thanks!

@nhosoi

nhosoi commented Dec 20, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for your approval, @Jakuje! Since I have no privilege to do so, if you could merge this pr, I'd appreciate it. Thanks!

@t184256 t184256 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, thanks.

@t184256
t184256 merged commit f5368d2 into linux-system-roles:master Dec 20, 2022
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