Add CI job to check coding style

[r0qs]

This PR adds a CI job to perform coding style checks as suggested here: #659 (comment)

It performs the eslint checks for .js and .ts files using the npm lint script and checks the .sh files using the shellcheck circleCI orb.

[cameel]

I have some minor suggestions but looks good overall.

[cameel]

A bit offtopic: I wonder if we could create custom orbs to make the CI config in the main repo more modular...

https://circleci.com/blog/building-private-orbs/

[r0qs]

Definitely! If there are many repeated tasks there that could also be reused by other repos in the organization, it would be beneficial to split them in orbs.

[cameel]

There is some common stuff. For example the gitter notification command may be a good candidate for an orb (while at it, we could also make it work on Windows). The t_ems_ext is used by external tests and their definitions are pretty repetitive too. And there are some common steps/templates. Also generally splitting the whole file into smaller bits by grouping similar jobs (build/test jobs, bytecode compare, external tests, etc.) would be great if possible.

We have a whole issue about that (ethereum/solidity#11846) but there are limits to what we can within a single file do and so far I did not consider creating orbs.

[r0qs]

Nice! Thanks, I can take a look on that :)
I can start with the gitter notification then, since there is already an issue for the failing solc-bin nightly builds.

[coveralls]

Coverage remained the same at 84.537% when pulling 86749f5 on ci-check-coding-style into 05a1955 on master.

[cameel]

Maybe shellcheck does need the ./ bit after all? I see in the CI output that it did not skip files inside node_modules:

./node_modules/tape/example/static/build.sh
./node_modules/foreground-child/changelog.sh
No ShellCheck Errors Found

Unless it just prints them without checking?

[cameel]

By the way, the orb is nice. I see that it for example automatically adds shellcheck.log as an artifact (if non-empty). That's handy.

[r0qs]

oh no! Indeed it is not skipping those directories. There is an example of the config in the official repo: https://github.com/CircleCI-Public/shellcheck-orb/blob/73e2427de6b0a5d0c27527b608ee62880e3dbfb0/.circleci/config.yml#L22 and the ./ is used. I will put it back.

[r0qs]

Hum...still it shows. So yeah, it seems that it parses anyway but probably don't check as you mentioned. So let's go without it :)

[cameel]

Can you try it out locally on a file where you intentionally make a mistake to make sure?

[r0qs]

Yes. It seems that it indeed considers the "ignored" folders when they terminate with / (e.g. node_modules/). If I remove the forward slash in the end, it works.

With node_modules/ or ./node_modules/:

./scripts/wrong.sh
./node_modules/foreground-child/changelog.sh
./node_modules/tape/example/static/build.sh
ShellCheck Errors Found

In ./scripts/wrong.sh line 5:
if [ $1 -eq "1"]; then
^-- SC1009: The mentioned syntax error was in this if expression.
   ^-- SC1073: Couldn't parse this test expression. Fix to allow more checks.
                ^-- SC1020: You need a space before the ].
                ^-- SC1072: Missing space before ]. Fix any mentioned problems and try again.

Without the forward slash in the end ./node_modules:

./scripts/wrong.sh
ShellCheck Errors Found

In ./scripts/wrong.sh line 5:
if [ $1 -eq "1"]; then
^-- SC1009: The mentioned syntax error was in this if expression.
   ^-- SC1073: Couldn't parse this test expression. Fix to allow more checks.
                ^-- SC1020: You need a space before the ].
                ^-- SC1072: Missing space before ]. Fix any mentioned problems and try again

In conclusion, we need the current directory prefix, and we should not add the end forward slash.

[cameel]

ok. Let's revert to the way it was originally.

Too bad that it's that picky. For a shell these would be perfectly fine and equivalent.

[r0qs]

Yes. I reverted it already. I opened an issue in the orb repo anyway, asking about this behavior.

[cameel]

So I guess we're done here? Looks ready to merge to me.

[cameel]

Oh, please squash the commits a bit though. Looks like 3 of the 4 commits are just review tweaks to the first one so they should be squashed into it.