New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linting mathjs #1110

Closed
harrysarson opened this Issue May 24, 2018 · 10 comments

Comments

Projects
None yet
3 participants
@harrysarson
Collaborator

harrysarson commented May 24, 2018

I would like to propose linting mathjs.

Using a tool such as https://eslint.org/ would make maintaining the libary easier, reduce the time spent reviewing pull requests and make contributing to mathjs an smoother experience.

We would need to decide on a configuration to use.

I guess the main downside would be that adding the linting would cause probably thousands of errors initially and would require a lot of work to get the linting passing. Maybe we should look for a way to incrementally lint. If we do add linting it would be nice to integrate it with our continous integration.

@josdejong josdejong added the feature label May 26, 2018

@josdejong

This comment has been minimized.

Owner

josdejong commented May 26, 2018

Thanks for your initiative!

I have no strong opinion about which linter we will use.

Qua rules, the current code base has semicolons, 2 space indentation. I think we should start with non-strict rules and over time make it more strict.

Besides linting, I would love to move the code base to ES6+, see #914, which involves setting up a tooling with babel/webpack/rollup/etc. I think it's good to keep that in mind: it could be that the setup we choose comes with a pre-baked solution for linting.

@firepick1

This comment has been minimized.

Collaborator

firepick1 commented May 27, 2018

A pass through an automatic formatter such as jsbeautifier might reduce the amount of effort involved in linting. Thank you very much for the proposal--linting is a labor of love and much effort.

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented May 27, 2018

Yeah, eslint comes with the --fix option which will fix indentation errors etc which would make the process a bit less painful.

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented May 31, 2018

I think we should start with non-strict rules and over time make it more strict.

An alternative would be to start with a full/strict set of rules and add /* eslint-disable */ to the top of each file, we could then remove it for one file at a time thereby slowly linting the whole of mathjs.

@josdejong

This comment has been minimized.

Owner

josdejong commented May 31, 2018

That's fine with me too! As long as we can migrate gradually I'm happy :)

@josdejong josdejong referenced this issue Jun 9, 2018

Closed

Breaking changes for v5 #1045

7 of 7 tasks complete
@josdejong

This comment has been minimized.

Owner

josdejong commented Jun 9, 2018

Anyone preferences for a JavaScript style?

I personally like to use 2 space indentation and no semicolons at line ends, so https://standardjs.com/ would be a nice fit.

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jun 9, 2018

Yeah, I like the idea of standardjs or xo that don't require tonnes of configuration

@josdejong

This comment has been minimized.

Owner

josdejong commented Jun 10, 2018

An easy to set up tool like standardjs or xo is a plus of course, but we can always switch to a different linting tool whilst changing the code base to a different style would involve a huge amount of work.

Most important to agree upon for me is the code style: standard, idiomatic, airbnb, node-style-guide, google, ... I think it will be good to choose a standard here rather than rolling our own favorite mix and match of styling choices.

I've experimented with standardjs, which is already close to the current code standard (except the semicolons and some spacing differences). It has an option to automatically fix most styling issues for you (npx standard --fix), and works out of the box running npx standard --env=mocha. Left over is only ~1600 issues, most of them are easy to fix.

Unless anyone has a strong opinion here I would like to go for https://standardjs.com/ .

@josdejong

This comment has been minimized.

Owner

josdejong commented Jun 12, 2018

Ok then now+tomorrow I'm migrating the code as much as possible to ES6, and I will release v5.0.0 tomorrow. I don't expect to work away all lint issues but hope I can fix most of them. We can do the rest later on bit by bit.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jun 13, 2018

It was a hell of a job but I managed to resolve all linting errors already, I hadn't expected that. About 12 hours of painstaking work, but really happy with the result. Linting is now running automatically when executing npm test 👍

All semicolons are gone. var is replaced with let/const. Most important work left over is to replace require with import, that's not so trivial.

@josdejong josdejong closed this Jun 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment