Run a spell checker during CI

lemmih commented 1 year ago

There are a lot of typos in the actor code: cirulating, acculumated, dosen't, etc. It would make the code look more professional if such typos were caught automatically. cargo-spellcheck can run a spell-checker on all doc comments.

jennijuju commented 1 year ago

@ElPaisano will take a stab on this

ElPaisano commented 1 year ago

Hello @lemmih 👋

Running into some initial install issues here. Any thoughts?

When I run cargo install --locked cargo-spellcheck to install per the instructions, everything works until the hunspell-sys v0.3.1 compilation step:

I'm not familiar enough with Rust to understand this issue.

Attempted troubleshooting:

Ensure hunspell is installed (it is, version 1.7.1)

System:

rustc 1.64.0
cargo 1.64.0
MacOS Monterey 12.6

ElPaisano commented 1 year ago

Whoops, the screenshot quality is pretty poor. Here's the error message:

error: failed to run custom build command for hunspell-sys v0.3.1

Caused by: process didn't exit successfully: /var/folders/6h/d00w5j7j59sbj5kmlrtc5_mh0000gn/T/cargo-install0qNVko/release/build/hunspell-sys-f142ecc4385acc05/build-script-build (signal: 6, SIGABRT: process abort signal) --- stderr dyld[18827]: Library not loaded: '@rpath/libclang.dylib' Referenced from: '/private/var/folders/6h/d00w5j7j59sbj5kmlrtc5_mh0000gn/T/cargo-install0qNVko/release/build/hunspell-sys-f142ecc4385acc05/build-script-build' Reason: tried: '/var/folders/6h/d00w5j7j59sbj5kmlrtc5_mh0000gn/T/cargo-install0qNVko/release/deps/libclang.dylib' (no such file), '/var/folders/6h/d00w5j7j59sbj5kmlrtc5_mh0000gn/T/cargo-install0qNVko/release/libclang.dylib' (no such file), '/Users/jssanchez/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libclang.dylib' (no such file), '/Users/jssanchez/.rustup/toolchains/stable-aarch64-apple-darwin/lib/libclang.dylib' (no such file), '/Users/jssanchez/lib/libclang.dylib' (no such file), '/usr/local/lib/libclang.dylib' (no such file), '/usr/lib/libclang.dylib' (no such file) warning: build failed, waiting for other jobs to finish... error: failed to compile cargo-spellcheck v0.12.3, intermediate artifacts can be found at /var/folders/6h/d00w5j7j59sbj5kmlrtc5_mh0000gn/T/cargo-install0qNVko

lemmih commented 1 year ago

@ElPaisano Looks like you don't have clang installed. For installation trouble, open an issue for cargo-spellcheck.

ElPaisano commented 1 year ago

@lemmih a thought:

the docs team is also working on integrating a linter into our markdown docs called https://github.com/errata-ai/vale. It checks spelling, grammar and writing style. Out of curiosity, I ran vale against a local copy of balance_table.rs. The output is as follows:

A few notes:

The leftmost column indicates the issue location <line>:<positition>
You'll see that vale caught spelling errors (examples at line 15, line 81)
Vale also flagged things that we wouldn't want to flag as spelling errors (examples at line 4, 5, 11)
Vale is completely configurable, meaning we could just specify in some config files that certain IPFS-specific and rust-y things should be ignored (fn, addr, function names like make_map_with_root_and_bitwidth, terms like cid and Blockstore, etc.) using accept lists, regex patterns, etc.
Vale can be used on the command line (what I did here) and as a GH action that runs on, say, every PR.
Vale can also check writing style against a style guide like https://github.com/errata-ai/Microsoft (the MSFT manual of style). In the output above, anything with an entry in the left column that begins with Microsoft. was a violation of one of those writing rules. Once again, this is configurable, so we could turn this on or off, or change the error level, use another style guide, etc.. If you guys wanted to also check writing style for comments, etc.

If you'd like to see an example of Vale as a GH action with a custom config, see https://github.com/ipfs/ipfs-docs/pull/1339 (WIP)

Anyways, lmk if you'd be interested in pursuing the use of vale instead of cargo-spellcheck. Other benefits would be that, because the docs team is already using this tool / we are going to be installing it in other doc sets, we are familiar with it and can help you guys with it

anorth commented 1 year ago

I'm on the fence about checking spelling, but game to try it as experiment. However I'm pretty confident that more general language linting will be a huge distraction from actual productive work. At worst, it will motivate engineers not to write comments.

lemmih commented 1 year ago

@ElPaisano I'm not interested in vale and I'm not entirely sure why you asked. I just reported a bunch of typos; how you want to fix them is up to you.

ElPaisano commented 1 year ago

@lemmih from the PR title Run a spell checker during CI and your initial comment https://github.com/filecoin-project/builtin-actors/issues/853#issue-1453405741, was under the impression that y'all were looking for help adding in an automated spell checker, which is what vale and cargo-spellcheck do. Please clarify if you are looking for typo fixes. We don't fix typos in the code base at the moment, that's something you and your engineers will need to do.

ElPaisano commented 1 year ago

@anorth as a bare minimum experiment, you and the team could use vale locally with a really simple spell check config and without any of the other grammar / writing style stuff prior to committing a change to the code base. No need to add it as another GH action.

So, the workflow would be:

Make an update to some rust file <some-file>.rs
Run vale <some-file>.rs
Fix any spelling errors that appear in the output

So, spelling errors would be fixed prior to commit and push. LMK if you want to try this.

filecoin-project / builtin-actors

Run a spell checker during CI #853