How to lint on the command line

Technical Support
1

Hi there!

I was helping out on a PR where I was told:

FYI, you can run the linting locally via the npm run lint command. :+1:

After some poking around through the dev setup page, I finally got npm run lint to run successfully! Then I was sad to see the result:

✖ 452 problems (452 errors, 0 warnings)
  445 errors and 0 warnings potentially fixable with the `--fix` option.

Out of curiosity, I checked out latest master and called the command again:

✖ 380 problems (380 errors, 0 warnings)
  374 errors and 0 warnings potentially fixable with the `--fix` option.

So I have a couple of questions:

  • how do I run npm run lint on just my PR’s changes?
  • should I be worried about the 380 errors on main in my branch?
  • is there a better way I should run this?

To the last point, I do see that there’s a couple of lines in the docs about eslint. Should I try to run the plugin in my IDE (Webstorm) instead?

potentially fixable with the --fix option

Also about this - I see when calling npm run lint --fix it doesn’t work. I assume the --fix flag is something you should pass into eslint directly or something? I don’t actually know how package.json works though… I can’t call eslint --color --cache . --fix directly - I get command not found: eslint :thinking:

1 Like
2

To unblock myself, I looked into the CI run to see the list of errors that needed linting on on my PR (mainly adding a space: {now: → { now: ; )

Update: Follow up to unblocking myself, a biiiit off topic, but still on the topic of linting, so I hope the judge will allow it:

I made a commit with a bunch of fixes for the linting errors. When I look at line 107 of the CI it says:

/home/runner/work/cht-core/cht-core/shared-libs/rules-engine/test/integration.spec.js
 107:39  error  Missing space before value for key 'now'  key-spacing

And checking the CI log it says it checkout the branch at hash d818bd748f945ee80e2c732e432facf457f492cb. Looking at integration.spec.js in that hash on github, it indeed shows there is a space before now:

 beforeEach(() => {
   clock = sinon.useFakeTimers({ now:TEST_START});
 });

so how do I get CI to run on my latest commits? I did try and re-run the job a second time and it failed in the same way. I’ll try it again with a new commit to see if that helps.

Update 2: Doing a 2nd commit (and a merge master → branch too) did not fix it : (

1 Like
4

Can’t we add linting check at pre-commit with husky? It will help us to check beforehand only before committing to codebase

5

This is a problem. Linting should pass for master. Possibly you did not update the dependencies you have installed locally by running npm ci? (And also, can try running npm run build-dev for good measure as that will help clear out and reset everything).

When checking out a new branch I always start by running npm ci to install the proper dependencies. This should be done before running any other npm scripts.

Technically there is no way to do this. We always require linting to pass before merging to master. So, any linting errors you see are caused by your branch (or hypothetically from a dependency issue caused by not running npm ci).

You are 100% correct here. The npm lint script in the package.json runs several commands and so you cannot pass --fix directly when running npm run lint (or at least I don’t know how).

This is also unexpected (and perhaps a result of not running npm ci? :thinking: ). Maybe there is some weirdness going on with your shell/path/node? After npm ci, the eslint binary should be accessible at ./node_modules/.bin/eslint (and whereis eslint resolves to that location for me in my shell). If eslint is still not resolving in your shell, maybe try running ./node_modules/.bin/eslint directly.

This is possible and something worth discussing if many folks think it would be valuable. Personally, I have found it much more useful to just setup eslint in my IDE (as noted by the docs linked above) so that I get the linting errors as I type instead of waiting until I try to commit.

6

Thanks for the detailed answer @jkuester, much appreciated!

I see know that I did run npm run build-dev but was speed running the setup and multiple times ignored the error I got at the end. I think this is because it just said warning and not error

mrjones.build-dev.log.pdf (58.7 KB) (sorry - you can’t post txt or zip files :confused: )

Entire verbose output is above, but I think the very last output is the relevant one - what do you think? It comes from the copy static file script:

copy-static-files: copying
cp: will not overwrite just-created 'api/build/static/admin/fonts' with 'webapp/src/fonts'
7

Fair point @jkuester I think it would be useful for newcomers or for those who haven’t gone through documentation for setting up linting locally.

8

WOW!!! Thanks so much for the help @jkuester ! We just spoke and figured that the cp that is being used in CI, and most likely most folks are using, is the this one:

➜  ~ /usr/bin/cp --version
cp (GNU coreutils) 9.5
Copyright (C) 2024 Free Software Foundation, Inc.

Which is, as it says, based on GNU Coreutils cp.

However, I’m running Bluefin and my cp is different:

➜  ~ /home/linuxbrew/.linuxbrew/opt/uutils-coreutils/libexec/uubin/../../bin/ucp --version 
ucp 0.0.30

Which is the Rust re-write called uutils coreutils:

uutils coreutils is a cross-platform reimplementation of the GNU coreutils in Rust. While all programs have been implemented, some options might be missing or different behavior might be experienced.

We have a PR out to fix this, so as a work around you can use the script from that branch, or use GNU Utils cp. An easy way to do the latter is to set your path to prefer GNU. Assuming this is in /usr/bin/cp, you would call:

export PATH=/usr/bin/cp:$PATH
1 Like
9

As a follow up to this issue, it was because I don’t really know JS :sweat_smile: In response to the eslint error Missing space before value for key 'now' key-spacing, I had made this fix :

 beforeEach(() => {
   clock = sinon.useFakeTimers({ now:TEST_START});
 });

However, that’s not actually what it wants. What it wants is a space between the value that is typed as now: (I think?). So the fix that finally passed linting was this:

 beforeEach(() => {
   clock = sinon.useFakeTimers({now: TEST_START});
 });

You can see all these fixes to make linting pass in in bce3b820a.