run eslint --fix

[hkollmann]

I suggest to run eslint lib --fix to do one time fix of all fixable lint problems. I did a test. After this eslint found 535 errors and 9 warnings, see appended errors.txt. We could easily correct this one by one whenever we work on that file.

What do you think?

errors.txt

[derrell]

Yes, please.

[hkollmann]

The fix will change nearly each file so that you have to do a clean checkout after.

[cboulanger]

Do you mean git pull doesn't work after that commit?

[cboulanger]

But it's a good idea to auto-fix trivial lint problems and deal with the rest later. Then I can finally turn on ESLint in my IDE!

[johnspackman]

I'm all for fixing the code, but this can only be done automatically for warnings, is that correct? I would prefer to fix the 535 errors by hand, despite the numbers.

[hkollmann]

@johnspackman: You are right - the 535 errors must be fixed manually. @cboulanger: sorry misunderstood, git pull will work. Only local changes will bring a lot of conflicts.

Should i prepare a PR?

[johnspackman]

While using lint can be a good thing, I think it needs ot be used with care - by default it can be far too strong minded and arbitrary. Reading that errors file, I can see a number of errors in my own code but I do not agree with a number of the rules (and think that they may be stronger than the core rules)- eg:

C:\_vza\@repos\github.windows\qooxdoo-compiler\lib\qx\tool\compiler\app\Library.js
  210:45  error    'cb' is already declared in the upper scope      no-shadow

IMHO renaming every instance of cb (which is a very common name for callbacks) to something unique like cb1, cb2, cbForSomeReason is not productive and does not provide any benefit.

C:\_vza\@repos\github.windows\qooxdoo-compiler\lib\qx\tool\compiler\app\Cldr.js
47:13  error  'parents' is already defined      no-redeclare

Well, ok I can see this might be useful for readability but it's legal code and I'm finding difficult to imagine situations where dropping the additional var is a big improvement.

C:\_vza\@repos\github.windows\qooxdoo-compiler\lib\qx\tool\compiler\Analyser.js
   504:29  warning  Do not nest ternary expressions      no-nested-ternary

Why not?

C:\_vza\@repos\github.windows\qooxdoo-compiler\lib\qx\tool\compiler\Analyser.js
   156:7   error    Unexpected negated condition      no-negated-condition

(I think that this should be line 150, not 156)

The no-negated-condition blocks if statements which are negated if they have an else condition - I think this is arbitrary and unhelpful. I lay my code out to make the most sense and ease of reading, and I do not agree that if (abc) {} else {} is necessarily any more clear than if (!abc) {} else {} because it depends on the context.

1010:9   error    This line has 2 statements. Maximum allowed is 1      max-statements-per-line

This line number is way out so I can't see exactly what it's referring to but ... Sigh.

C:\_vza\@repos\github.windows\qooxdoo-compiler\lib\qx\tool\compiler\app\Application.js
236:28  error  The Function constructor is eval      no-new-func

This is intentional, and the reason why new Function() is used in the first place ...

C:\_vza\@repos\github.windows\qooxdoo-compiler\lib\qx\tool\compiler\app\Translation.js
  133:21  error  'entry' was used before it was defined      no-use-before-define

eslint either does not understand hoisting or elects to block it. IMHO hoisting is fundamental feature of javascript and it's acceptable to use it; in this particular instance moving the var up a bit isn't a big deal, just irritating, but there are other places where (imho) moving the declarations will not improve readability and would need careful review.

C:\_vza\@repos\github.windows\qooxdoo-compiler\lib\qx\tool\compiler\ClassFile.js
777:27   error    'VISITOR' was used before it was defined      no-use-before-define

This is a better example of when no-use-before-define is particularly unhelpful - there are various state machines that go on here that need to be switched between, and they are currently arranged in the most logical order, taking advantage of the benefits of hoisting.

[cboulanger]

We should definitely disable the rules @johnspackman has problems with. They go beyond what is necessary for good, qooxdoo-style code. Maybe instead making a list of rules that we WANT to have?

[hkollmann]

rules defined here: https://github.com/qooxdoo/eslint-config-qx. I tried to define them as near to the core rules as possible. Some input was taken from @derrell a year ago.

Except for no-redeclare i would agree to delete the others.

[johnspackman]

That's fine by me

[cboulanger]

Me too. We need to update the documentation here: https://github.com/qooxdoo/eslint-config-qx#handling-globals - it says "qxc for contribs", whereas it should be "qxl for external qooxdoo libraries and components" (or how do we refer to the qxl.* libraries?

[hkollmann]

qx lint loads all libraries as globals automatic.

[johnspackman]

I'll start work on the qx.tool.compiler.* classes

[johnspackman]

Also, this "error":

/Users/john/dev/Qooxdoo/QxCompiler/lib/qx/tool/compiler/utils/Values.js
  29:51  error  Trailing spaces not allowed                                                   no-trailing-spaces

That's literally a couple of trailing spaces at the end of the line - my editor (Eclipse) does that all the time when auto indenting.

[hkollmann]

Will be fixed by --fix!

[hkollmann]

IMHO we should do eslint --fix first! Otherwise you will get a lot of conflicts!

[hkollmann]

I could prepare a PR or i can do it directly on master if you agree to do so.

[cboulanger]

+1 for eslint --fix first directly into master

[derrell]

I had created a lint configuration file. We had a lot of discussion about it, but we each had things that were important to us that conflicted with others' strong desires. I believe, for example, that all variables should be declared at the top of the scope in which they apply, so that there is a short chain of locations to look for declarations, rather than all over the code. @johnspackman likes to spread his var statements all over the place, which I find unreadable, but he prefers. And that's just one example. The compiler is a separate project from qooxdoo, and should follow the coding conventions of those working on it (even though they're wrong :-) :-) :-) ) We just can't, IMO, run "fix" on the qooxdoo code itself, until we come up with a team-agreed lint configuration.

[johnspackman]

Lets wait before changing it again, there are some more issues below:

/Users/john/dev/Qooxdoo/QxCompiler/lib/qx/tool/compiler/utils/Values.js
  37:28  error  Unexpected space before function parentheses      space-before-function-paren

My personal preference is to not have spaces between the function name and parentheses, but this is pretty-printing minutia to me.

/Users/john/dev/Qooxdoo/QxCompiler/lib/qx/tool/compiler/utils/Values.js
1:1   error  Expected space or tab before '*/' in comment             spaced-comment

Similarly, is this really necessary? Who actually cares if there is a space before the last */ in the header block comment?

38:1   error  Expected indentation of 6 spaces but found 8                                  indent

Different editors have their own interpretation of how far to indent open functions vs objects

@hkollmann

eslint --fix first!

OK, but what a PITA it will be to have to reformat every time we commit! It means that it is just about impossible to edit without running eslint --fix to reformat.

I'm in favour of a largely similar coding style without strict enforcement - I think that's reasonable because qooxdoo core has developed over time with multiple styles, and we have our own styles which are all highly readable. IMHO lint is to help us avoid common coding mistakes.

[cboulanger]

ok - I correct myself: what about eslint --fix into a branch to see what the changes are? They should only concern really trivial things like unnecessary spaces at the end of the line.

[cboulanger]

and don't you think that we CAN find a core of common rules, leaving out those on which there is no consensus? Maybe the lowest common denominator is quite small but concerns rules that make our lives easier with contributed code? Because I think that contributed code -and not our own code - is our main use case for (es-)lint.

[johnspackman]

@derrell as I recall, this was based on one of your projects but we didn't settle on anything? My recollection is that we agreed to follow the existing standard first, which would be ./generate.py lint (not that I agree with all of that either! 😁 )

WRT spreading var to where it's needed, like a lot of coding it depends on context and IMHO lint rules can be very dogmatic in their interpretation. Now that we have ES6 and let, then block scoping is part of the language and I suspect that there is less potential harm in that style. But to me, it reminds me of Pascal (one project of I still have to maintain), and in reality I think it is 50/50 value of your way vs mine ...

I think that using lint to enforce styling is primarily useful when you have a team of varying skills and experience, and there are benefits to enforcing strict coding standards and styles (especially on the younger members of the team). But we're all pretty experienced coders here, and the only grotty looking code I've seen in Qooxdoo is some more recent commits by 1&1 coders in the later days.

Also in a team, you can mandate that everybody uses the same editor with the same auto-formatting settings - whereas we're using Eclipse, Emacs, IntelliJ, at the least.

@cboulanger

and don't you think that we CAN find a core of common rules

Yes I do, provided we're cautious (eg especially styling standards)

[hkollmann]

@johnspackman: "But to me, it reminds me of Pascal (one project of I still have to maintain), and in reality I think it is 50/50 value of your way vs mine …"

Newest version of Delphi supports inline variables :-)

[hkollmann]

I spend some work a year ago to have the eslint-config-qx package as near as possible to generate.py lint.

[johnspackman]

Newest version of Delphi supports inline variables :-) Bliss! I have a Delphi 7 project that runs in an XP VM :shudder: :wink:

I spend some work a year ago to have the eslint-config-qx package as near as possible to generate.py lint.

I imagine that we're nearly there, I was just a little surprised to find some of the stylistic restrictions. I havn't had time to look at this before.

Don't get me wrong, there's a large chunk of that 535 errors that I would definitely consider errors and I'm embarrassed to admit that they're mine!

I'm working through the compiler code now.

[hkollmann]

After fixing the rules as suggested above there are only 346 problems. Result of --fix is in hkollmann-lint branch now.

[cboulanger]

Most effective would probably be to have a list of eslint rule ids and descriptions and then vote by unanimity which to keep. But I don't know about tool which would enable this fast & painless. What about, for example single-vs-double quotes? I don't have a strong opinion and would follow a rule either way as long as it can be auto-fixed, but is it really important?

[cboulanger]

Here's the diff for @hkollmann's lint branch: https://github.com/qooxdoo/qooxdoo-compiler/compare/hkollmann-lint?expand=1

[johnspackman]

Here's my branch with all of qx.tool.compiler.* lint warnings and errors fixed, baring a few choice rules which I've disabled. I'll write up the docs on which rules later, plus fix the unit tests which are failing, before submitting as a PR

https://github.com/johnspackman/qooxdoo-compiler/tree/compiler-lint

[johnspackman]

The compiler code is in PR https://github.com/qooxdoo/qooxdoo-compiler/pull/276 to match the PR in https://github.com/qooxdoo/eslint-config-qx/pull/1

I found that eslint --fix really helpful but for some reason it also broke my indentation in a lot of places, returning the code back to column 1 that had to be manually fixed.

[hkollmann]

Done with https://github.com/qooxdoo/qooxdoo-compiler/pull/284