builtin-actors icon indicating copy to clipboard operation
builtin-actors copied to clipboard

Run a spell checker during CI

Open lemmih opened this issue 2 years ago • 9 comments

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.

lemmih avatar Nov 17 '22 14:11 lemmih

@ElPaisano will take a stab on this

jennijuju avatar Nov 28 '22 15:11 jennijuju

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:

image

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 avatar Nov 28 '22 16:11 ElPaisano

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

ElPaisano avatar Nov 28 '22 16:11 ElPaisano

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

lemmih avatar Nov 29 '22 09:11 lemmih

@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:

image

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

ElPaisano avatar Nov 29 '22 19:11 ElPaisano

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.

anorth avatar Nov 29 '22 21:11 anorth

@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.

lemmih avatar Nov 29 '22 21:11 lemmih

@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 avatar Nov 30 '22 00:11 ElPaisano

@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:

  1. Make an update to some rust file <some-file>.rs
  2. Run vale <some-file>.rs
  3. 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.

ElPaisano avatar Nov 30 '22 00:11 ElPaisano