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 renaming advice based on IETF draft-knodel-terminology-00 #1556

Merged
merged 4 commits into from
Jun 11, 2020

Conversation

hadess
Copy link
Contributor

@hadess hadess commented Jun 10, 2020

Based on:
https://tools.ietf.org/id/draft-knodel-terminology-00.html#rfc.section.1.1.1

Offer to replace master/slave with:

  • Primary-secondary
  • Leader-follower
  • Active-standby
  • Primary-replica
  • Writer-reader
  • Coordinator-worker
  • Parent-helper

@hadess
Copy link
Contributor Author

hadess commented Jun 10, 2020

The CI error is:

E               AssertionError: error mater: correction master is an error itself in the same dictionary file dictionary_rare.txt

I wonder whether we have any way to say "you meant this, but you probably want to use something else anyway".

@larsoner
Copy link
Member

Hmm, this one is a bit trickier. master in particular is not rare/being pushed out of usage (AFAIK) -- in particular it's the default branch on GitHub so this is probably not something we want enabled by default.

This almost seems like something that should have its own dictionary so that people can opt into using. dictionary_knodel.txt maybe?

@hadess
Copy link
Contributor Author

hadess commented Jun 10, 2020

Hmm, this one is a bit trickier. master in particular is not rare/being pushed out of usage (AFAIK) -- in particular it's the default branch on GitHub so this is probably not something we want enabled by default.

Based on my research, the master of the main git branch is an orphan reference to BitKeeper's "master/slave" repositories for replication, so seems like a good candidate for obsolescence.

There's a fairly good chance that other "engineering" uses of the word "master", such as "master copy", would also be orphan references like this one, but I lack the research skills to dig down.

@larsoner
Copy link
Member

Based on the number of false alarms (i.e., "I want to keep using master for my main branch name at least for now") people will experience, I don't think we should enable these by default. Hence why I think it should go into a separate opt-in dictionary. (Same might actually be true of the list variants we just added.)

@hadess
Copy link
Contributor Author

hadess commented Jun 10, 2020

Based on the number of false alarms (i.e., "I want to keep using master for my main branch name at least for now") people will experience

I don't really see a difference between "I want to keep using master for my main branch name at least for now" and "I want to keep this typo for compatibility reasons". It might take time to work-around and eventually fix, but it's not a "false alarm".

@hadess
Copy link
Contributor Author

hadess commented Jun 10, 2020

I'd be happy extending the code or the checks to allow for 2 levels of "correction", if you tell me how you'd want it to behave.

@larsoner
Copy link
Member

I don't really see a difference between "I want to keep using master for my main branch name at least for now" and "I want to keep this typo for compatibility reasons". It might take time to work-around and eventually fix, but it's not a "false alarm".

What you are proposing is a word usage change -- in this sense, a standard typo like adn (adn is not a word) is different from master (master is a word, but perhaps shouldn't be used). So perhaps "false alarm" is not the right term, but there is a difference between what you are proposing and standard typos.

One important consideration is that this change will cause a lot (probably a majority!) of our users to need to add a new argument to codespell like -L master, which is not good in terms of backward compatibility (and annoyance at a previously green CI turning red). Especially since this is a suggested usage change rather than an actual word misspelling, I'd rather have such a disruptive change be in an opt-in list rather than force a ton of our our users to have to add a new argument to get their old behavior back, even if I do personally agree that it would be better to change this usage community-wide. You could argue that perhaps users should need to do this to have it brought to their attention, but I'd (again) rather make it opt-in, and make an effort to widely advertise that people should enable it and adopt the new language, rather than force it on them at a time not of their choosing (i.e., whenever we choose to push a new codespell to pip and suddenly everyone's CIs is red because master is used everywhere on GitHub).

BTW we use whitelist in the codespell README.rst, feel free to update this to the newer language...

@larsoner
Copy link
Member

I'd be happy extending the code or the checks to allow for 2 levels of "correction", if you tell me how you'd want it to behave.

That is exactly what adding a new dictionary would do, we just wouldn't have it in the default set and it would be opt-in. Maybe dictionary_usage.txt would be appropriate, since it's changing suggested usage? Then it would be added to this list:

https://github.com/codespell-project/codespell/blob/master/codespell_lib/_codespell.py#L39

And it wouldn't be enabled by default, since it's not in the default list clear,rare

@hadess
Copy link
Contributor Author

hadess commented Jun 10, 2020

Understood, I'll update the patch to use a separate dictionary that tries to include the other suggestions from the IETF draft as well.

@larsoner
Copy link
Member

Probably worth moving whitelist/blacklist there, and then also enabling that dictionary where we test our own repository (though we'll need to keep master at least for now because changing the primary branch name would need to be a separate PR)

@peternewman
Copy link
Collaborator

While I agree with the principal of this and #1554 , I think they should be in another dictionary file entirely (which can be enabled by default if deemed appropriate). I think the original typos and corrections should exist in the dictionaries they are currently in, and we probably need to add a new option so this special dictionary is ignored from the list of corrections.

From what I can tell the patch for #1554 doesn't appear to have been merged into Linux Git (unless I'm looking in the wrong place).

Also similar to the master stuff, there may be external requirements to use those terms for now, e.g. if you are spell-checking stuff includingAPIs or config for external code which uses those terms.

I think informal is the equivalent, there are lots of nuanced reasons why people may still have to (even if they don't want to), use these words currently.

@larsoner
Copy link
Member

larsoner commented Jun 10, 2020

I don't think informal is quite equivalent, I'd rather have a new dictionary_usage.txt for things that track community-accepted (possibly time-varying) usage standards

@peternewman
Copy link
Collaborator

I don't think informal is quite equivalent, I'd rather have a new dictionary_usage.txt for things that track community-accepted (possibly time-varying) usage standards

Yeah sorry, I didn't mean putting them in that dictionary, but neither set are typos, they are changes in usage. You may want to allow or not for various reasons.

@hadess hadess force-pushed the wip/hadess/master-slave branch 3 times, most recently from 2e56992 to cd0da1a Compare June 11, 2020 10:40
@hadess
Copy link
Contributor Author

hadess commented Jun 11, 2020

Apologies for the spam, my distro doesn't have aspell packaged up, and I thought I could get away without using a virtualenv...

@hadess hadess force-pushed the wip/hadess/master-slave branch from cd0da1a to c0d497f Compare June 11, 2020 10:54
Copy link
Collaborator

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

A few comments @hadess .

codespell_lib/tests/test_dictionary.py Show resolved Hide resolved
codespell_lib/_codespell.py Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
hadess added 2 commits June 11, 2020 13:55
Add a new "usage" dictionary with usage advice based on:
https://tools.ietf.org/id/draft-knodel-terminology-00.html#rfc.section.1.1.1

Offer to replace master/slave with:
- Primary-secondary
- Leader-follower
- Active-standby
- Primary-replica
- Writer-reader
- Coordinator-worker
- Parent-helper
Backout the changes from commit 98ab0fa and move them into the
"usage" dictionary.
@hadess hadess force-pushed the wip/hadess/master-slave branch from c0d497f to 5f28353 Compare June 11, 2020 11:55
Use the recommended replacement "allowlist"
@hadess hadess force-pushed the wip/hadess/master-slave branch from 5f28353 to dca2068 Compare June 11, 2020 11:56
Copy link
Collaborator

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @hadess .

@larsoner larsoner merged commit 5ed84fc into codespell-project:master Jun 11, 2020
@larsoner
Copy link
Member

Thanks @hadess !

@larsoner
Copy link
Member

Next step I think would be to make sure we enable this dictionary in testing our own repo at least. Probably will require adding a -L master somewhere, other than that I think we should be able to make the other changes easily

@peternewman
Copy link
Collaborator

Next step I think would be to make sure we enable this dictionary in testing our own repo at least. Probably will require adding a -L master somewhere, other than that I think we should be able to make the other changes easily

I've made a start on this here codespell-project/actions-codespell#14 just need to fix some of the tests.

@hadess
Copy link
Contributor Author

hadess commented Jun 12, 2020

Thanks @hadess !

Thanks for being a receptive project :)

@peternewman
Copy link
Collaborator

Probably will require adding a -L master somewhere

Not for much longer: https://www.bbc.co.uk/news/technology-53050955

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.

3 participants