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 new CI Spellcheck job #6297

Merged
merged 12 commits into from
Jan 29, 2024

Conversation

Owen-CH-Leung
Copy link
Contributor

As per #6263 , This PR introduce a new CI spellcheck job using cargo spellcheck. The job leverages the spellcheck.toml and spellcheck.dic to perform spelling check on the docs.

add spellcheck.dic, spellcheck.toml, and add new CI spellcheck job
@Owen-CH-Leung Owen-CH-Leung marked this pull request as ready for review January 18, 2024 15:04
@maminrayej maminrayej added the A-ci Area: The continuous integration setup label Jan 18, 2024
@maminrayej
Copy link
Member

maminrayej commented Jan 18, 2024

I think it makes sense to surround Rust and tokio specific words with backticks. This would significantly reduce the size of the .dic file, and make it more maintainable. Also, we don't have to fight the spellchecker each time a new struct, method, etc is introduced.
Some examples of Rust specific words: &str, &mut, ...
Some examples of tokio specific words: AsHandle, AsyncWrite, ...

Additionally, it would be beneficial to incorporate a section in CONTRIBUTING.md that outlines the spell checking process. Similar to existing sections on rustfmt, clippy, and testing, this new section could provide guidelines on running the spell check command, updating the dictionary, and documenting in a way that aligns with spellchecker requirements, such as the use of backticks.

@Owen-CH-Leung
Copy link
Contributor Author

Owen-CH-Leung commented Jan 19, 2024

maminrayej

Thanks for your feedback. So you mean we should modify Rust and tokio specific words inplace so that we don't have to add the words in spellcheck.dic ?

e.g. &mut in https://github.com/tokio-rs/tokio/blob/master/tokio/src/runtime/time/entry.rs#L85-L88 should be changed inplace to `&mut`

@maminrayej
Copy link
Member

Yes. In that particular example I think &mut TimerEntry is more reasonable. Basically anywhere referring to code I believe it should be enclosed in backticks because it will most probably be flagged by hunspell.

@Owen-CH-Leung
Copy link
Contributor Author

Ok make sense to me. It'll require some more time to modify each comment piece by piece though. I'll revise them soon

@maminrayej
Copy link
Member

No worries. Thanks.

@github-actions github-actions bot added the R-loom-time-driver Run loom time driver tests on this PR label Jan 23, 2024
backticks common rust / tokio words, and fix some typos
@github-actions github-actions bot added R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR R-loom-sync Run loom sync tests on this PR labels Jan 25, 2024
fix failing doc CI
@Owen-CH-Leung
Copy link
Contributor Author

@maminrayej Can I ask for your review again ? I backtick-ed lots of rust / tokio words and trimmed the dictionary file down to 3xx entries.

However, I notice that not all rust / tokio words can be backtick-ed. (e.g. AsRawHandle cannot be backtick-ed and removed since it's part of the doc test in here. And I don't think we can configure cargo spellcheck to skip checking the doctest

@maminrayej
Copy link
Member

Great progress!

The issue you've mentioned happens because of a formatting mistake in that file. There are two empty lines that are commented using // instead of ///. One at line 2062, and the other one at line 2097. I believe this confuses cargo spellcheck. If you fix these two lines, that file can be successfully spellchecked with this small .dic file:

1
dwOpenMode
Tokio
tokio
runtime
Runtime
syscall
Dev
wakeup
waker
async
qos
sync
unsafety

I've tested this using the following command:

cargo spellcheck check tokio/src/net/windows/named_pipe.rs

Keep an eye out for these formatting mistakes if you encounter something like this again. When you apply these fixes, the size of the .dic file could be further reduced.

CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
…vise the contributing.md

Further trim down the dic size, revise a few formatting error, and revise the contributing.md
add newline for ci.yml, cargo.toml, spellcheck.dic, spellcheck.toml
spellcheck.dic Outdated Show resolved Hide resolved
spellcheck.dic Outdated Show resolved Hide resolved
spellcheck.dic Outdated Show resolved Hide resolved
spellcheck.dic Outdated Show resolved Hide resolved
spellcheck.dic Outdated Show resolved Hide resolved
spellcheck.dic Outdated Show resolved Hide resolved
spellcheck.dic Outdated Show resolved Hide resolved
spellcheck.dic Outdated Show resolved Hide resolved
Backticked more words & trim down dictionary
@Owen-CH-Leung
Copy link
Contributor Author

Thanks @maminrayej for your comment. I've backticked some more words and removed them from the dictionary. However, there're still a few words which I'd say it's just normal english words and I'd recommend putting it into dictioary (e.g. amongst (here), Blockingly (here), broadcasted (here), cancelled(here), functionalities(here) etc...)

@maminrayej
Copy link
Member

Ah I think it's a misunderstanding. GitHub shows around the place where a comment is placed. For example in here:

scheduler's
semver
setpgid
shard_id

Comment is placed on shard_id, but a few lines above it is also displayed. I just meant that shard_id is code related.

Let me give you the list of words here:

assert_eq
bool
buf
cfg
const
fuzz_linked_list
shard_id

Everything else should be reverted back. Sorry for the inconvenience.

…rom dictionary

remove assert_eq, bool, buf, cfg, const, fuzz_linked_list, shard_id from dictionary
@Owen-CH-Leung
Copy link
Contributor Author

assert_eq
bool
buf
cfg
const
fuzz_linked_list
shard_id

Thanks. Reverted everything and removed only these 7 words

@Owen-CH-Leung
Copy link
Contributor Author

Can you also trigger the failing test to rerun ? I saw the cross-test-without-parking_lot was failing but I don't think my changes break the codebase...I suspect it's just intermittent failure

@maminrayej
Copy link
Member

Thanks!

@Darksonn I believe this PR is in good shape. There are additional work that can be done like making sure the .dic file is ordered and it doesn't contain duplicates. But I think it's better to do it in a different PR.

@Darksonn Darksonn merged commit 131e7b4 into tokio-rs:master Jan 29, 2024
82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ci Area: The continuous integration setup R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR R-loom-sync Run loom sync tests on this PR R-loom-time-driver Run loom time driver tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants