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

Run a spell checker during CI #853

Open
lemmih opened this issue Nov 17, 2022 · 9 comments
Open

Run a spell checker during CI #853

lemmih opened this issue Nov 17, 2022 · 9 comments
Labels

Comments

@lemmih
Copy link
Contributor

lemmih commented Nov 17, 2022

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
Copy link
Member

@ElPaisano will take a stab on this

@jennijuju jennijuju moved this to 📋 Backlog in Network v18 Nov 28, 2022
@ElPaisano
Copy link

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
Copy link

ElPaisano commented Nov 28, 2022

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
Copy link
Contributor Author

lemmih commented Nov 29, 2022

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

@ElPaisano
Copy link

ElPaisano commented Nov 29, 2022

@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 ipfs/ipfs-docs#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
Copy link
Member

anorth commented Nov 29, 2022

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
Copy link
Contributor Author

lemmih commented Nov 29, 2022

@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
Copy link

ElPaisano commented Nov 30, 2022

@lemmih from the PR title Run a spell checker during CI and your initial comment #853 (comment), 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
Copy link

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

@anorth anorth added the P3 label Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: 📋 Nice To Haves
Development

No branches or pull requests

4 participants