Skip to content

Fix typos and replace 'codespell' with 'typos'#72

Merged
PingXie merged 1 commit intovalkey-io:unstablefrom
jayvdb:fix-typos
Mar 31, 2024
Merged

Fix typos and replace 'codespell' with 'typos'#72
PingXie merged 1 commit intovalkey-io:unstablefrom
jayvdb:fix-typos

Conversation

@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Mar 29, 2024

fle = "fle"
nd = "nd"
ot = "ot"
module_gil_acquring = "module_gil_acquring"
Copy link
Contributor Author

@jayvdb jayvdb Mar 29, 2024

Choose a reason for hiding this comment

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

this is in an extern variable type, so I didnt want to rename it without discussion.

However, especially given valkey is still unstable, maybe this is a good time to do the rename.

Does valkey want to retain API or ABI compatibility?

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 see #43

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this isnt listed in the codespell wordlist.


/* Make the thread killable at any time, so that kill threads functions
* can work reliably (default cancelability type is PTHREAD_CANCEL_DEFERRED).
* can work reliably (default cancellability type is PTHREAD_CANCEL_DEFERRED).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://en.wiktionary.org/wiki/cancelability and the note at the bottom of https://www.merriam-webster.com/dictionary/cancel suggests this was an acceptable alternative , but given it only appears once, and it looks like it will get flagged by multiple spellcheckers, it seems wise to just switch to using the spelling commonly used.

@szepeviktor
Copy link
Contributor

szepeviktor commented Mar 29, 2024

Please consider dropping codespell.
Its dictionary is included in typos: https://github.com/crate-ci/typos/tree/master/crates/codespell-dict

@PingXie
Copy link
Member

PingXie commented Mar 29, 2024

Please consider dropping codespell. Its dictionary is included in typos: https://github.com/crate-ci/typos/tree/master/crates/codespell-dict

I am not familiar with codespell nor spellchecker in general. Are you saying that the existing spellchecker is sufficient?

BTW, @jayvdb, all other changes LGTM. Thanks!

@jayvdb
Copy link
Contributor Author

jayvdb commented Mar 29, 2024

I did a bit of digging and can confirm typos is a superset of codespell wrt to misspelling detection capability, and finer grained configuration. codespell has a few UX features that typos doesnt have, such as an interactive TUI mode. Anyone wanting those features can continue to use codespell locally.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

LGTM

How can I run typos locally? It's useful to be able to run it before pushing, instead of waiting for CI failure to find out.

@zuiderkwast zuiderkwast changed the title Fix typos Fix typos and replace 'codespell' with 'typos' Mar 30, 2024
@jayvdb
Copy link
Contributor Author

jayvdb commented Mar 30, 2024

LGTM

How can I run typos locally? It's useful to be able to run it before pushing, instead of waiting for CI failure to find out.

See https://github.com/crate-ci/typos?tab=readme-ov-file#install

@szepeviktor
Copy link
Contributor

How can I run typos locally?

Just download typos, it is a single binary. Nicely fits in ~/bin/
https://github.com/crate-ci/typos/releases

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Sounds good then.

I'm leaving this open for a little while to let others look before merging.

@zuiderkwast zuiderkwast added the to-be-merged Almost ready to merge label Mar 30, 2024
Signed-off-by: John Vandenberg <jayvdb@gmail.com>
@PingXie
Copy link
Member

PingXie commented Mar 31, 2024

Looks like we are in general agreement with the bulk of the changes. I am going to merge this PR now to reduce the PR backlog. We can always take incremental changes.

@PingXie PingXie merged commit 253fe9d into valkey-io:unstable Mar 31, 2024
@PingXie PingXie added polish and removed to-be-merged Almost ready to merge labels Mar 31, 2024
madolson added a commit that referenced this pull request Apr 7, 2024
These were flagged on the 7.2 build system, which is using the old spell
check. I think we should consider re-adding it as it missed some typos.

Relevant: #72

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
madolson added a commit to madolson/valkey that referenced this pull request Apr 7, 2024
These were flagged on the 7.2 build system, which is using the old spell
check. I think we should consider re-adding it as it missed some typos.

Relevant: valkey-io#72

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this pull request Apr 24, 2024
Uses https://github.com/taiki-e/install-action to install
https://github.com/crate-ci/typos in CI

This finds many more/different typos than
https://github.com/codespell-project/codespell , while having very few
false positives.

Signed-off-by: John Vandenberg <jayvdb@gmail.com>
PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this pull request Apr 24, 2024
These were flagged on the 7.2 build system, which is using the old spell
check. I think we should consider re-adding it as it missed some typos.

Relevant: valkey-io#72

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
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.

5 participants