feat(chore): add lint using CI

[mvorisek]

Description

Unified CS improves readability, prevents hidden mistakes and enforce unified contribution standards. And in a long term, it also simplifies merge conflicts.

The CS rules are based on industry standard eslint-config-airbnb-base, but all rules might be too hard to fix at once. This PR focuses on whitespace CS which can be autofixed /wo having to review each line manually.

related #29

nicely integrated with Github Actions like:

[prudho]

Yes, code linting is definitely something we need to implement. I started using myself eslint at work and this is something i don't regret at all. But... 😅

We cannot implement it, and already use it to enforce merge requests. I think that first we need to define which rules we want to use and the level of error it'll raise. Then we can do some merge requests to fix the code. But we can't fix all the code in one PR, maybe a PR per component.

Once this is done, then we can enforce lint check trough the CI pipeline.

[mvorisek]

I have updated the PR, instead of using airbnb-base as a base eslint config (with possible side effects) I added the most important whitespaces rules only to match general coding standards. With whitespace changes the code behaviour is not changes and this PR is safe to merge.

In another PR, more eslint rules can be added.

Also don't worry about merge conflicts (from changes based on code before this PR), they can be easily resolved, just cherry pick the eslint rules and run npm run lint-fix, you can then integrate the changes as usual.

[lubber-de]

As the PR is basically ok now, i just comment instead of requesting changes as i would like to hear opinion of other maintainers. See my comments below

[lubber-de]

I took a look at the complete files to get myself convinced about the 4 indent 😉 and i recognized the linter does sometimes not implement the 4 indent correctly, which needs (manual?) adjustments, otherwise the new indent is making it worse than before at those codelines, because it gets confusing

For example dimmer.js, look at the semicolon, which terminates the var statement

Fomantic-UI/src/definitions/modules/dimmer.js

Line 38 in 9074205

;

It is now looking like that

while it was and is supposed to look like that (same indent as the var statement)

[mvorisek]

Maybe we need another eslint rule for semicolon, I will have a look on a solution for it.

[mvorisek]

The solution is to run eslint semi: ['error', 'never'] once and then again. There is no explicit config of indentation for semicolon in eslint.

Multiline expr. is indented correctly, thus IMHO it is better to have the semicolon on the same/last expr line, as the 2nd (and following) multiline expressions are indented visually already.

related issue: eslint/eslint#15752 (the solution with semi: ['error', 'never'] fixes it too)

[lubber-de]

Well, when adding the semicolon to the end of the last line, why the indention at all then (in such cases)?

Also, you added the rule to add a trailing comma in object literals. The linter idea behind that is to easily exchange/adding/removing single lines without the need to also add another comma to the previous attribute notation.
This is the same for such use cases where the semicolon has its own line.

So: Adding the trailing comma but also adding a semicolon to the last entry is not consistent anymore (IMHO even worse)
I would really prefer to have the semicolon being indented as it was before. This clearly shows where the indentation block ends as it is now.

Now we have the discussion i basically want to avoid ;) And now imagine new contributors trying to help and we are going to argue about "correct" whitespacing :)

Don't get me wrong, I am not against the linting. But indentin the semicolons would be the cleaner approach. Yes, this possibly needs a (onetime) manual adjustment if there is no 100% automatic solution.

[mvorisek]

Well, when adding the semicolon to the end of the last line, why the indention at all then (in such cases)?

chained method call indentation is defacto standard (at least has always been in all codebase I have worked with) and it makes sense, as

$module
    .callOne()
    .callTwo();
$module.callOne();

is IMHO visually more readable and more compact than

$module
.callOne()
.callTwo()
;
$module.callOne();

Don't get me wrong, I am not against the linting. But indentin the semicolons would be the cleaner approach. Yes, this possibly needs a (onetime) manual adjustment if there is no 100% automatic solution.

You are right here, I have reviewed the changes and there are at least two usecases with semicolon on a NL:

a) ternary:

I have never seen semicolon on NL for them, also no reason for easier diffing, as ternary operator is always binary.

b) var declaration:

I will fix these manually 👍

[exoego]

For the record, I am positive on introducing linter.
I believe it helps both contributors and maintainers.
I defer to lubber on styling preference since he is the most active maintainer.

[lubber-de]

$module
    .callOne()
    .callTwo();
$module.callOne();

is IMHO visually more readable and more compact than

$module
.callOne()
.callTwo()
;
$module.callOne();

My point basically was about comparison to the trailing comma rule.
If you want to add a .callThree() you now have to remove the semicolon from .callTwo() as well

I will fix these manually 👍

Thx 😊

[mvorisek]

PR is done, I added more whitespaces rules to match AirBnB CS (basically the rules are copied from their ruleset). The hash of minified semantic.min.js file before and after this PR is unchanged, ie. there is no functional change.

Later, I plan to create another PR which will fix var -> let etc., which can have side effects.

[lubber-de]

LGTM! Thanks for doing this and your patience 😉