Enforce a style guide

[magopian]

Fixes #77

To run eslint:

• for node: npm run lint-node (use npm run lint-node-fix to auto fix some issues)
• for the browser: npm run lint-browser (use npm run lint-browser-fix to auto fix some issues)
• for both: npm run lint (use npm run lint-fix to auto fix some issues)

[magopian]

Node files: https://irccloud.mozilla.com/pastebin/YV5kJB7X/eslint_node.txt
Browser files (7MB file): https://irccloud.mozilla.com/file/4zHxSmy2/eslint_browser.txt

[jankeromnes]

Wow, yet another really cool contribution. Thanks a lot!

I had a bunch of questions about these configurations, please see them below :)

Also, I'm curious if/how we'll be able to lint files in templates/, since they're HTML but contain some custom templating code (e.g. {{= title in html}}).

[jankeromnes]

Interesting, what are these eslint:recommended and plugin:node/recommended rules? Also, are they not already included in standard?

[magopian]

I believe those are base rules that are "cascading"? Here's the eslint:recommended: http://eslint.org/docs/rules/

I'm not sure which is where, I just feel it's sensible to do it like that, but we could very well just keep standard I guess?

[jankeromnes]

I don't get these parser options for the browser environment. Since we don't use ES6, why add "ecmaVersion": "2017"? Also, I don't believe we do any experimental object rest spread or JSX here (which is React's non-standard JS format).

[jankeromnes]

Nit: These are actually rules that contradict standard.

[jankeromnes]

Nit: We try to use dangling commas on multiline objects and arrays. Please use something like "comma-dangle": ["error", "only-multiline"] (or you may want to explicitly list all options in order to special-case "functions": "never", because it may break older browsers).

[magopian]

not in the browser, as discussed on irc. Or did I misunderstand?

[jankeromnes]

We don't use exports or module.exports in the browser.

[jankeromnes]

Interesting, why did you add these?

[magopian]

mmmm, copy pasted from the html profiler devtools, i'll remove them.

[jankeromnes]

Why did you add all these? Aren't they examples already present in "standard"? Also a few of them are ES6+ and don't make sense in the browser.

[magopian]

same as above

[jankeromnes]

Why set "browser": true in server files?

[jankeromnes]

Thank you for fixing this bogus whitespace!

[magopian]

I added some automatic git hooks that will run the linting before each commit and push. You'll need to run npm install to activate them.

When commiting or pushing you should see something like this (check the little spinner next the "lint-browser-fix" line):

If the checks fail, you'll be prompted with the following message:

husky > pre-commit hook failed (add --no-verify to bypass)

So if you wish to bypass the checks, simply use git commit --no-verify ....

EDIT: reverted and added in a separate PR: #104

[jankeromnes]

Heh, @nt1m is not going to like this 😛

[nt1m]

Noooooooooo :D

[nt1m]

Can you leave libs unchanged and add a .eslintignore file including common library patterns ?

[magopian]

What kind of "common library patterns"? Keep in mind I barely know any JS ;)

[nt1m]

static/js/*.min.js as you figured out :)

[magopian]

ah, that, sorry, didn't understand your statement at first ;)

[magopian]

Added an .eslintignore file, and ignored all files ending with .min.js. There aren't many errors left, and the linting step before commiting/pushing is not really short (less than a second on my machine), which is imho totally acceptable.

[jankeromnes]

This is amazing! Thanks a lot for fixing all these issues 😄

A few random nits and questions, but we're really close to being able to merge this. Please just punt commit/push hooks to a follow-up pull request (or drop them for now), as there doesn't seem to be consensus on their value yet.

Also, once you're done, I might shove a few additional commits on top of yours, in order to quickly fix-up occurrences of function myFunc () {} exports.myFunc = myFunc; to exports.myFunc = function () {}.

[jankeromnes]

Nit: Please add an empty line at the bottom of this file in order not to offend the Git.

[jankeromnes]

Nit: Please finish the file with an empty line.

[jankeromnes]

Nit: These rules should be in the browser rules, not in the Node.js/server rules. In the node rules, it should be 'command-dangle': ['error', 'only-multiline'].

[magopian]

iiuc the browsers are not all on board with allowing dangling commas, that's why i have https://github.com/JanitorTechnology/janitor/pull/101/files#diff-a75257962af8dc9ccbfdb1d2c43be2feR13 which disables those checks. But then maybe I should rewrite this to "comma-dangle": "off" instead, it would be clearer I guess.

Regarding node, it does allow dangling commas, right? So that's why I have "only-multiline", but as we discussed on irc, only for objects and arrays, nothing else, or did I misunderstand? So appart from rewriting the browser rule to be clearly "off", what else do you want me to modify?

[jankeromnes]

Interesting. This return was indeed unnecessary, but I added it as an indication that there should be no further instructions in this block (below, there are some inline functions, but there shouldn't be any other code or logic that risks calling response.json() or response.end() a second time). This is just a random remark, I'm fine with this change.

[jankeromnes]

Nice catch! 😄

[magopian]

It was an auto-fix <3

[jankeromnes]

Huge 👍! I used to do all these changes by hand... You're saving me so much time!

[magopian]

Same here, just an auto-fix

[jankeromnes]

Actually, I'm not fond of commit hooks, and @nt1m is worried about them too. Do you mind removing them from this pull request, and maybe suggest them in a follow-up pull request if you really think they're really valuable?

[jankeromnes]

Nit: I believe we should allow dangling commas for multi-line objects and arrays in browser code.

[magopian]

Wow, so I did completely misunderstand our discussion on IRC. My understanding was that browsers don't all allow them, but node does. Can you please confirm that it's the opposite?

[jankeromnes]

So, most browsers support:

var a = {
  b: 4,
  c: 5,
};

And:

var a = [
  c,
  d,
];

But only very recent browsers support:

function bla(
  a,
  b,
  c,
) { }

So in browser code, we want to forbid dangling spaces in function arguments (as well as in imports/exports since they don't make sense in a browser env anyway).

In Node.js code, we can add dangling commas everywhere (i.e. on every multi-line thing), even function parameters.

[magopian]

I believe I changed it to do just that, if I'm wrong, please let me know ;)

[jankeromnes]

Please don't delete this function! I know it's not used yet, but we might want to use it in the future.

[magopian]

Ok I'll revert this commit ;)

[jankeromnes]

👍!

[nt1m]

I think you can use something like import-globals-from or define something in static/.eslintrc.js instead of hardcoding the globals

[magopian]

i believe I've addressed all your comments (but #101 (comment), should I do something about that?).

Please let me know if there's anything else you wish me to modify ;)

[jankeromnes]

Excellent progress! This is really cool, although it will break all other pull requests and force them to rebase 😛 (not an issue at all though).

However, the snake_case to camelCase auto-fix breaks destructuring like let { b_c } = a; and "restructuring" (is that an actual word?) like return { b_c };. Note: both are syntactic sugar for let b_c = a.b_c; and return { b_c: b_c };.

[jankeromnes]

Nit: Please either log('[fail] something something') or throw new Error('Something something'), but not both. (In this case, I believe you want to just throw, and thus there is no need for the [fail] log prefix).

[jankeromnes]

Nit: What? Why?

[magopian]

It's my understanding that those are needed to fix http://eslint.org/docs/rules/no-case-declarations

[nt1m]

Can you simply move the contents of url inside routes.redirect and remove the extra variable and the brackets?

[jankeromnes]

Why change this to a String? The Node.js fs.chmod documentation says that the mode argument should be an Integer (even though it does unofficially support strings). There is no example, but the default value is said to be 0o666 in other places.

[magopian]

The octal syntax seems to be deprecated, see http://eslint.org/docs/rules/no-octal (first paragraph). In strict mode, eslint will complain about a parsing error.

[nt1m]

I prefer that we use 0o666 rather than '0600'.

[jankeromnes]

Grmbl... fair enough. But we might as well make these const, by enabling the prefer-const ESLint rule.

[jankeromnes]

Nit: In general, log messages are lower-case, while error messages in new Error()s are capitalized.

Nit 2: Please add the word client as in while getting docker client.

[jankeromnes]

Nit: Please remove the word Error, e.g. like so '[fail] problem while reading stream'.

[jankeromnes]

Nit: -Error, -the.

[jankeromnes]

Nit: Maybe '[fail] could not create session'?

[jankeromnes]

Nit: Is it really worth it to have separate lint-node{,-fix} and lint-browser{,-fix} commands? Why not just have lint and lint-fix?

[magopian]

You're right, I'll remove the others ;)

[jankeromnes]

So, most browsers support:

var a = {
  b: 4,
  c: 5,
};

And:

var a = [
  c,
  d,
];

But only very recent browsers support:

function bla(
  a,
  b,
  c,
) { }

So in browser code, we want to forbid dangling spaces in function arguments (as well as in imports/exports since they don't make sense in a browser env anyway).

In Node.js code, we can add dangling commas everywhere (i.e. on every multi-line thing), even function parameters.

[magopian]

I believe I've addressed your comments, please let me know if I've missed anything.

I added the no-var rule in af4933d, please have a deep look at my manual changes there, to make sure I haven't set something as const while it should really be a let ;)

I also rebased the PR, so I hope I didn't break anything in the process :/

[magopian]

Rebased following latest PR merges

[nt1m]

Quite funny that the eslintrc.js files don't actually respect the eslint rules :D

Property names should either have no quotes or single quotes.

[magopian]

I believe it's because you can either use a js file, or plain json? I don't have an opinion on this matter. Also for some reason those configuration files are auto-ignored by eslint.

[nt1m]

Almost there :)

[nt1m]

Can you simply move the contents of url inside routes.redirect and remove the extra variable and the brackets?

[nt1m]

It's inconsistent that we have return; in the above case statement, but not here. Can you instead use break; here and above to remove this inconsistency?

[nt1m]

I prefer that we use 0o666 rather than '0600'.

[nt1m]

Same here.

[nt1m]

You can use:

// eslint-disable-next-line node/no-unsupported-features
const machines = Object.values(user.machines)

[nt1m]

Nit: please use // eslint-disable-next-line instead.

[nt1m]

nit: I prefer you use /* exported formatMemory */ on top of the file

[magopian]

that doesn't seem to change eslint's mind about warning on this line :/

Probably because of eslint/eslint#1200 (comment)

[nt1m]

nit: please use

// eslint-disable-next-line no-func-assign
done = null;

I think it's cleaner as it doesn't clutter up the line length.

[nt1m]

nit: use eslint-disable-next-line

[nt1m]

nit: please use // eslint-disable-next-line

[magopian]

@nt1m and @jankeromnes i'm not sure I've addressed all your comments, this PR is getting so long that it's hard to tell. Please let me know if I've missed anything.

On a side note, I did change the eslint exceptions to use // eslint-disable-next-line but I'm not sure it's a good idea: it's the kind of comments which will

• have a much bigger importance (at the same level as code comments explaining pieces of the implementation, and adding "noise", instead of just being an addition far to the right of the line
• be easily forgotten if the line that is being ignored is moved around, or fixed, or removed

[nt1m]

have a much bigger importance (at the same level as code comments explaining pieces of the implementation, and adding "noise", instead of just being an addition far to the right of the line

I think eslint-disable-next-line looks cleaner as it doesn't make the line too long. I do think having a line that's suddenly wider than the other ones looks odd. I would even be favorable to forbidding long inline comments.

be easily forgotten if the line that is being ignored is moved around, or fixed, or removed

Either eslint will catch the mistake, either the comment is no longer relevant :)

[jankeromnes]

Approved!

With a few very minor nits for the road, because I couldn't resist. 😛

[jankeromnes]

Nit: This will break the tests I think.

[magopian]

Yeah, I don't know where this change comes from, I believe it was from a rebase? I personally didn't make this change, do you want me to revert it?

[nt1m]

Yeah, I think it would be preferable to revert it

[jankeromnes]

Nit: This url variable is not used by anyone else than the routes.redirect() below. Please inline its value directly into the redirect call (no need for an intermediary variable).

[jankeromnes]

Nit: I believe you meant { content += String(chunk); } instead of (content += String(chunk)) (we didn't actually want to return the results of the +=).

[jankeromnes]

Nit: Please alpha-sort NPM scripts (except for pre* and post* scripts).