Linting and codeing style

[mojoaxel]

We should discuss about introducing linting and enforcing some coding style.

[mojoaxel]

@AoDev introduced linting with #2695

[mojoaxel]

The reason we did not use linting until now is mainly because it would destroy the "blame" history and some contribution statistics. I personally planned enabling linting and enforcing some code style wit the next real big break. Such a break could be the splitting of the project (#2405).

[AoDev]

Actually, the reason I have decided to send a PR is because there is an eslint file in the repo. But, the dependencies are not installed and so my editor was crying because it could not find eslint to lint the code.
While I think linting is definitively necessary, and it also helps find bugs, if you think that it's not the right time for it, I'd remove the eslint file from the repo.

But when you have PR it's better to lint because then there will be useless discussions about style in every PR. Who said semi-colon? :P

[mojoaxel]

While I think linting is definitively necessary, and it also helps find bugs, if you think that it's not the right time for it, I'd remove the eslint file from the repo.

I'm totally with you on the benefits of linting, but I'm not sure about the right moment to change a big part of the existing code.
Maybe we can start with a very minimal linting and enroll a much more strict linting with the v5 release!?

[AoDev]

I often use standardjs in my projects or in our company.
There are also other solutions like prettier but, I haven't used this one.
There will always be a few rules that go against personal tastes but in general one get quickly used to it and the point is not having decisions based on personal taste to avoid discussions XD.

[controversial]

I personally like airbnb/javascript. It's a pretty unopinionated set of rules for eslint that mainly just enforces correct, modern javascript, with a few small style rules (camelCase, etc.)

[wimrijnders]

I'm for everything that makes the code better, and linting is a great idea.

But I am aware that introducing linting in one go is going to generate a lot of errors, and hence a lot of work to correct this. I hereby propose adding linting gradually.

I've been reading up on eslint; I believe that it's possible to disable linting by default by using the Configuration Cascading and Hierarchy. Then, linting can be enabled for the specific code that you want to check:

... Ignore code here

/*eslint-enable*/

  // Check this code!

/*eslint-disable*/

... Ignore code here too

In this way, linting can be enabled stepwise, until all code is being checked.

---

I like this idea also because it allows contributors to make their changes as good as possible.
I recall that when I started contributing, there was a links to these coding conventions in the contributing guide, which I thought was an excellent idea. This has been replaced with the, IMHO, rather lame:

Always adapt to the code style of the existing source. Never adapt existing code to your personal taste.

I would rather give contributors an option to improve according to the standards that should exist. A message along the line of:

When adding code, if you adapt your code style to the existing source, we're good with that.
However, if you care enough to make the code as good as possible, you can do this as well:

And then a list, like:

• Style conventions
• jsdoc
• enable linting

[AoDev]

A lot of rules are auto fixable. I don't think it will be such a hassle to update. You can simply turn on a bunch of rules little by little and auto fix. If a rule will require more work, it can be done later.

[wimrijnders]

I suppose that is a valid approach. Still, there will be sweeping changes through the 100+ files even if they only are small. I would think a more restrained approach would be advisable.

[AoDev]

Why do you feel uncomfortable with it? In case it breaks some things?

[wimrijnders]

To give a direct answer: yes. I believe that is part of the job of being a maintainer.

Another thought I have is that doing a full adjustment according to linting output may leave the code in an interim state for a while until it has been done. This would result in complications due to parallel submissions, needing extra work to consolidate.

Also, see my second point above. I would like a possibility for contributors to adhere to the guidelines that should be there.

[AoDev]

It can't be done without planning, that's for sure. But only maintainers know what's going on. Usually if a change affects the whole code, we tell people that there will be a code freeze. So we merge the PRs etc, freeze, do our maintenance and then resume.

[wimrijnders]

OK, good to know. The above was the personal thought I wanted to share, if there are other good ways of doing it, that's fine with me.

[mojoaxel]

I propose the following:

• Let's improve tests so we have a better feeling about big code changes. (I'm working on that)
• Let's have a short vote which Ruleset we want to use. There are were already some mentioned.
• Let's ask the big committers (@josdejong and @AlexDM0) if they are ok with the change.
• Let's do all the initial code changes by an neutral user like "visBot" or something similar.
• Let's enforce the new rules in the build process and the travis ci checks for future pull requests.

OK?

[wimrijnders]

OK by me. All sounds sensible.

[mojoaxel]

Oh, this is nice. It looks like we could do the change without loosing the git history (my main concern at the moment):
https://stackoverflow.com/questions/4112410/git-change-styling-whitespace-without-changing-ownership-blame

[AlexDM0]

I think the airBnb one looks like a step in the right direction, except for the iterators part. While more readable, there is a severe performance hit. For anything backend I'd still use for (let i...) because of this. The 15.3 also seems odd. I'd prefer explicit checks always, even for booleans. Simple reason is that sometimes those can be undefined due to typos etc. (happened so many times in vis :))

Keep up the great work guys!

On a sidenote;
I've seen typescript pass by before, I've been using it a lot recently and it has been great (albeit a bit slow). I'd be all for making vis typescript by default (just rename all js to ts) and have people add typing when they either find a bug or write new code.

[mojoaxel]

I started a poll to find a coding style: #2904
PLEASE EVERYBODY PARTICIPATE!!

[wimrijnders]

I'm with StandardJS for the most part. I found only two minor things in the features list I disagree with. And even those I can live with.

[mojoaxel]

I prefer much more the more "old school" style like jquery or Wikimedia.

Like every other programmer I'm very opinionated when it comes to indentation. I'll never understand using multiple spaces when you clearly want to use a tab 😉

But I'm completely open for everything that makes sense!

[controversial]

I'm a fan of Airbnb's config personally. I haven't really encountered much that I don't agree with.

[mojoaxel]

I'm a fan of Airbnb's config personally. I haven't really encountered much
that I don't agree with.

The AirBnB is a "hard-core" ES6 style. I'm not sure if we really want to change all the code to ES6...But is is obviously widely used and very "modern".

[yotamberk]

Why not? ES6 should be integrated. It really isn't that different than JS.
I'm really against TypeScript @AlexDM0 ... It seems like a hype too me... I really don't think it should be the default for vis.

[bradh]

No hard preference, but would prefer not to invite our own (or modify someone elses significantly). Just pick something.
The only thing against ES6 (or anything else we transpile) is that it doesn't lend itself to standalone unit tests very well. There is an old comment in the tests about it. No problem when the tests pass, just harder to debug when things go wrong, which is more friction in improving the test coverage.

[controversial]

It's ES6 is JS. It’s the next version of JavaScript, it’s a concrete standard with growing browser support uncompiled. No reason to keep using an older version of the language when we could start to improve code structure and fully take advantage of the power of the language

[josdejong]

Good idea to add a coding standard. Thanks for asking. I don't have a strong opinion on which standard is best, just having a standard is good :)

[mojoaxel]

I'm not sure If all of you understand what it means to enforce e.g. the airbnb style on the whole project!
We would have to rewrite almost every single line of code in the whole project to make it compatible.

Feel free to checkout the eslint branch and run npm run lint to get a feeling...

Once again: AirBnB is a ES6-only style. We would have to convert the whole code base to ES6.

[AoDev]

When I did the lint PR, I originally checked with "extends": "eslint:recommended" . Could be also an option, it is much less "revolutionary" than using other code styles, so less hassle to migrate to it.

Anyway if a project is started without enforcing a style, it's inevitable that the whole codebase will be affected once rules are imposed. I think it should be thought by looking at the expected goal. What's the goal: have a sets of rules that don't belong to anyone so that the code looks like written by a single person. Plus linting does save us from some bugs.

The style itself is almost irrelevant except that it should not get in the way of people contributions. Some people will never send a PR to a typescript / coffeescript project for example. If one wants ES6 code, it's not about style, it's about using new features of the language.

[mojoaxel]

When I did the lint PR, I originally checked with "extends": "eslint:recommended" . Could be also an option, it is much less "revolutionary" than using other code styles, so less hassle to migrate to it.

@AoDev Thx! Can you please add this option to #2904.

[mojoaxel]

wimrijnders : Scanned the list, @AoDev's suggestion is a pretty sane one to begin with.

@wimrijnders This is also the preset currently in place: https://github.com/almende/vis/blob/develop/.eslintrc#L11

From there we would only need to remove the special rules one by one..