Skip to content
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

Add tool to remove condescending language from LORIS documentation #5789

Merged
merged 9 commits into from
Feb 13, 2020
Merged

Add tool to remove condescending language from LORIS documentation #5789

merged 9 commits into from
Feb 13, 2020

Conversation

johnsaigle
Copy link
Contributor

Brief summary of changes

How to remove condescending language from documentation

This post outlines why condescending documentation is a bad idea and recommends the alex tool to fix it.

This tool should be helpful as we go forward in creating our documentation on ReadTheDocs.

This PR:

  • Adds the alex tool to devDependencies
  • Creates configuration files for this tool
  • Fixes detected occurrences
  • Adds alex to our CI pipeline to prevent future issues.

@johnsaigle johnsaigle added Category: Feature PR or issue that aims to introduce a new feature Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) Meta PR does something that organizes, upgrades, or manages the functionality of the codebase labels Nov 26, 2019
@@ -0,0 +1,4 @@
allow:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These terms are potentially insensitive when used to refer to people, but are helpful in the context of discussing code, e.g.

"invalid data"
"special characters"
"executed a command"

README.md Outdated
@@ -1,5 +1,6 @@
# [![Build Status](https://travis-ci.org/aces/Loris.svg?branch=master)](https://travis-ci.org/aces/Loris) LORIS Neuroimaging Platform

<!--alex ignore easy-->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The word 'easy' is not helpful to include in code instructions because different tasks are more difficult depending on your background.

However, it's a nice "sales" line in the context of this paragraph

PapillonMcGill
PapillonMcGill previously approved these changes Dec 2, 2019
Copy link
Contributor

@PapillonMcGill PapillonMcGill left a comment

Choose a reason for hiding this comment

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

As long as it only generate warning so it won't block as PR.
Nice addition to help us improve text for mishap.

@johnsaigle
Copy link
Contributor Author

I think the warnings do cause build failures actually. I imagine this is configurable.

I'm OK with having it cause build failures because it's quick to fix the errors and I don't think anyone will see them or fix them otherwise (e.g. the JS lint warnings).

@PapillonMcGill
Copy link
Contributor

Whaaatt? There are JS lint warning not resolved! You have a good point here.

I guess it wouldn't be difficult to had an exception in case some word are flag even if correct in the context.

@johnsaigle
Copy link
Contributor Author

Yes I agree. And if a word causes problems but we agree it's fine in context then we can add it to the exclude list there.

README.md Outdated
@@ -1,5 +1,6 @@
# [![Build Status](https://travis-ci.org/aces/Loris.svg?branch=master)](https://travis-ci.org/aces/Loris) LORIS Neuroimaging Platform

<!--alex ignore easy-->
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we need to start hacking up our documents to add machine annotation for this tool, I don't think it should be run automatically as part of Travis and it adds another dependency (both for developers and Travis). We should just be cognizant of the writing style during code review and someone can periodically run the tool to make sure nothing slipped through.

The link that you provided even says that it's intended to flag things for human review, not be used as an absolute rule.

As tempting as it is, you don't want to remove every instance that is flagged by alex or your own search. Because, sometimes, these terms can be present out of necessity or as an attempt to be more welcoming.

(That said, the few changes identified in this PR look good to me.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I see your point.

Maybe we could assign someone to run the tool during our release process?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense to me, as long as we remember

Copy link
Contributor

Choose a reason for hiding this comment

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

that's always precisely the issue. Do we have a new checklist for the release process, that we actually use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GitHub project functions as a kind of distributed checklist, I guess.

Anyway I've added a make target for this tool (not included as a part of Travis though) so that it's quick to run the check anyway. Still a step forward.

@johnsaigle
Copy link
Contributor Author

@driusan I added a commit that creates a separate make target for the alex script which also removes it from Travis. I also removed the annotation from README as it's no longer needed to pass Travis.

Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

when just means only (rather than simply) - it should not be stripped.
@johnsaigle i don't think this can be calibrated.

docs/SQLModelingStandard.md Show resolved Hide resolved
docs/SQLModelingStandard.md Show resolved Hide resolved
@johnsaigle
Copy link
Contributor Author

@christinerogers I agree with your points. I added some commits just now commits to reword the sentences to be more clear while avoiding the word just. Not that the word is bad in the context but I would like to be able to use the tool without ignoring all cases of the word 'just' or manually adding exceptions to these documents.

Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Would love to discuss this; I like the idea and I think this should definitely be accompanied by:

  • Add this link to our Contributing.md in a sentence like "Please note we try to avoid using certain language in our codebase, for details see this list", accompanied by:
  • a hotlink to the actual list of flagged words, for clarity (e.g. should...)

My $0.02 : We are making taboo a number of words with multiple meanings, not all of which are insensitive, e.g.

  • modules can simply connect via the API
  • the file you committed just now
    Yes we can find substitutes but why not just flag for validation in the commit process instead of expurgation?

docs/API/LorisRESTAPI.md Show resolved Hide resolved
@@ -193,7 +193,7 @@ b. Create the file `ext-xdebug.ini` with this command:
sudo touch /usr/local/etc/php/<your_php_version>/conf.d/ext-xdebug.ini
```

c. Type the following into the `ext-xdebug.ini` file you just created (Note: replace the `zend_extension` path with the correct one on your machine):
c. Type the following into the `ext-xdebug.ini` file you created (Note: replace the `zend_extension` path with the correct one on your machine):
Copy link
Contributor

Choose a reason for hiding this comment

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

Again -- this is not the use of just we're looking to expunge...

Copy link
Contributor Author

@johnsaigle johnsaigle Dec 3, 2019

Choose a reason for hiding this comment

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

3 approaches in this case:

  1. Reword the sentence
Suggested change
c. Type the following into the `ext-xdebug.ini` file you created (Note: replace the `zend_extension` path with the correct one on your machine):
c. Type the following into the `ext-xdebug.ini` file created by the above command (Note: replace the `zend_extension` path with the correct one on your machine):
  1. Add an exception like <!--alex ignore just-->, but Dave asked me not to do this.

  2. Let this be flagged each time the tool is run, generating noise in the report that we have no intention of fixing.

My favourite solution is the 1st.

- **Date** fields should not just be named “Date”. A
**qualifier** like “BoughtDate” or “DateAccepted” should be added as
required.
- **Date** fields must have **qualifers** (i.e. they should not be named “Date”). A
Copy link
Contributor

Choose a reason for hiding this comment

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

typo fix:

Suggested change
- **Date** fields must have **qualifers** (i.e. they should not be named “Date”). A
- **Date** fields must have **qualifiers** (i.e. they should not be named “Date”). A

@johnsaigle johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Dec 3, 2019
@johnsaigle johnsaigle changed the base branch from major to master December 3, 2019 17:41
@johnsaigle johnsaigle removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Dec 3, 2019
@johnsaigle
Copy link
Contributor Author

Add this link to our Contributing.md in a sentence like "Please note we try to avoid using certain language in our codebase, for details see this list", accompanied by:

I'd be open to a "We use a tool called Alex to help us create accessible and friendly documentation. For more details please view their GitHub repository at [link]"

a hotlink to the actual list of flagged words, for clarity (e.g. should...)

It uses various lists, including ones that contain e.g. racist slurs so I don't think we should do this. A link to the tool is sufficient imo.

modules can simply connect via the API

Is this really so simple though? I have 3 years of experience here and I'm not 100% confident I could explain to someone how to do this. I don't think "simply" adds anything to this sentence and functions at best as a buzzword and at worst as a source of frustration.

I agree with you it's not insensitive but the motivation of the tool aren't limited to words that are disrespectful. it's more subtle as well as more broad than that use case.

Rewording this kind of sentence would be an improvement for this reason.

Yes we can find substitutes but why not just flag for validation in the commit process instead of expurgation?

The idea here is to be able to run the tool and scan for problematic use of language. This tool will generate a report based on everything it thinks is an issue. In some cases these will be false positives, as you've noted and I agree with what you're saying.

At the same time, I'd prefer reword these few instances so that the report contains all signal and no noise when we use it down the line. If the tool is to be useful it shouldn't contain a lot of noise. An example is the ESLint warnings that have been in the repo for years now but nobody cleans up because there's no incentive to do so.

@johnsaigle
Copy link
Contributor Author

@driusan @christinerogers Could you please update your reviews?

@@ -66,3 +66,6 @@ This means using more direct language and tone to get information across to the
* Break up large blocks of text. It's more visually pleasing and less intimidating.

For more information, including research studies conducted, on the use and value of Plain Language, please see [this article](https://www.nngroup.com/articles/plain-language-experts/) and feel free to browse the rest of this website for further helpful details, tips, and tricks.

In addition to the above guidelines, we use [Alex JS](https://github.com/get-alex/alex)$
to help us create documentation that is friendly and accessible.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this advertisement for a third party tool belongs here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christinerogers asked that it be mentioned somewhere in the documentation. It felt most relevant to include it in a document outlining our documentation style.

I don't think it's an advertisement so much as just a fact about the project, similar to saying "we use phan to make sure our code doesn't suck"

Copy link
Contributor

Choose a reason for hiding this comment

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

"and if you want to know why we might change particualr language you use, here's the info... "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel that providing the link to the tool implies indicates that that's the place to find info.

Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

Even if I didn't have my last comment, this can't be merged because there's no Travis run for it.

Makefile Show resolved Hide resolved
@johnsaigle johnsaigle dismissed driusan’s stale review January 6, 2020 17:22

Changes addressed; travis is being run

@johnsaigle johnsaigle requested a review from driusan January 8, 2020 18:27
@johnsaigle johnsaigle added State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Jan 8, 2020
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

last minor nitpick

CONTRIBUTING.md Outdated
@@ -76,6 +76,8 @@ using the command `make checkstatic` and fix any resulting errors. Otherwise,
would affect the data or custom code of a study - document this in your pull
request description and tag it with **Caveat for Existing Projects**.
This helps us to document our release notes.
* In addition to the above guidelines, we use [Alex JS](https://github.com/get-alex/alex)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still shouldn't be advertising external tools. The HelpStyleGuide mention of it is fine, but for CONTRIBUTING.md the contributor should just need to know that they can run make checklanguage to verify the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@johnsaigle johnsaigle dismissed driusan’s stale review February 7, 2020 22:27

Changes incorporated

@johnsaigle johnsaigle requested a review from driusan February 7, 2020 22:27
@driusan driusan merged commit b95740f into aces:master Feb 13, 2020
@ridz1208 ridz1208 added this to the 23.0.0 milestone Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) Category: Feature PR or issue that aims to introduce a new feature Meta PR does something that organizes, upgrades, or manages the functionality of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants