Feature/eslint

[cdaringe]

problem statement

• linting is a little out-of-date!

solution

• address choose a new linter #11
• add git-validate, which installs a generic git pre-commit hook
  • add pre-commit tasks in package.json, particularly the "lint" task
  • remove the old pre-commit script
• make files comply with new .eslintrc.js

discussion

i'd really like a "zero configuration" eslint setting, in that regard we never debate or discuss opinions on rules. but arrrg, no-unused-vars was really killin me! if we're all cool with this, just as it stands, great, we can use it and not make a fuss. or, if someone wants to try another standard, no-sweat!

[paulcpederson]

@cdaringe how did you commit without the tests passing? Isn't the pre-commit hook supposed to catch that?

[cdaringe]

@paulcpederson, because i didn't realize i needed to update the .travis.yml too. bumping...

[paulcpederson]

word

[Lochlan]

Why change the travis config to npm run lint instead of just updating the make lint target? At the very least I would remove the now-dead make lint, but IMO it should stay in place and get updated. If you like being able to npm run lint you can also just pass that through to make lint once updated.

Full disclosure: I am somewhat obsessed with GNU Make, it's one of my favorite tools.

[paulcpederson]

@cdaringe I think I agree with @Lochlan on that. I'd leave make lint and update it to run the new linter (https://github.com/node-swig/swig/blob/master/Makefile#L90). Then you could just do:

"scripts": {
  "lint": "make lint"
}

There is quite a bit of stuff in the Makefile and unless we wanted to do away with Make entirely, it would be cleaner to keep all the tasks run through make IMO.

[paulcpederson]

Also, I had a question, does this maintain linting for indentation/punctuation errors?

[cdaringe]

Why change the travis config to npm run lint instead of just updating the make lint target?

i want to run the lint task automatically on pre-commit s.t. all contributors don't forget to lint their code manually. i could surely use the make command, but

• it's extra overhead (i.e. runs an extra child process vs running the eslint directly), and
• adds an extra level of indirection
  • runs a command purely as a proxy to another command npm => make => eslint vs just npm => eslint
  • npm scripts are already acting as a task runner--why sustain a redundant secondary task runner?

I am somewhat obsessed with GNU Make, it's one of my favorite tools

make is great, but IMHO it's not friendly or common in the js community. if this were a C++ library, id say rock on, but using js standards common js practices in a js lib seems kind of like an "of course" sort of thing to me. i would lobby that make is not as friendly to new contributors.

it's sort of bikeshedding in the grand scheme of things. anyway, hopefully we can come to resolution

[paulcpederson]

I appreciate that argument, that it's not node-like. If we go ahead with moving towards npm scripts we should clean up the stuff we don't need the Makefile to do now, though. Like remove https://github.com/node-swig/swig/blob/master/Makefile#L88 through line 92.

Also you could remove the githooks stuff here https://github.com/node-swig/swig/blob/master/Makefile#L16 right?

[Lochlan]

if this were a C++ library, id say rock on

Yes, Make is essentially designed for C and C++, but ultimately it is a declarative language for describing the relationships between files. And it comes out of the box on pretty much every unix/linux machine!

i would lobby that make is not as friendly to new contributors.

Makefiles can be arcane, but generally recipes contain a lot of straightforward shell commands. That's one of the things I love about Make, is its accessibility on the low end.

using common js practices in a js lib seems kind of like an "of course" sort of thing to me.

I certainly agree with you here in the sense that using JS build tools is the most common practice, though I don't think that conforming to common practices is always necessary or beneficial.

Despite my gushy love of Make, I'm not actually opposed to using a JS build tool. I agree with @paulcpederson re: "I appreciate that argument, that it's not node-like."

npm scripts are already acting as a task runner--why sustain a redundant secondary task runner?

Because where we are today is that we already have the makefile. As @paulcpederson said:

There is quite a bit of stuff in the Makefile and unless we wanted to do away with Make entirely, it would be cleaner to keep all the tasks run through make IMO.

I'm very leery of moving to a state where we have some parts of our build inside the makefile and some outside. And I'm definitely not wanting to leave in dead recipes in the makefile. Perhaps there can be a separate issue and PR for discussing a new build tool? At this exact moment in time, and for the actual scope of this work (moving to a new linter), I would prefer to just update the make lint recipe and keep things simple.

I definitely think the makefile as-is has problems and we should consider some alternative/improvement to what exists currently.

[Lochlan]

I also want to state that this is merely my preference, and that I consider moving the lint target out of the makefile a pretty minor thing. I would not want this trend to continue, however, unless/until moving to a new build tool.

[paulcpederson]

Yeah this gets my +1. Let's open a new issue to discuss the build tool and get this merged.

[paulcpederson]

nice work @cdaringe 👍

[cdaringe]

restored to make. should have had that discussion before starting a change. best to do it all at once if at all! i originally didn't consider it scope creep as we already sorta-kinda use both, but that's arguably grey!

hopefully travis get's its act together and gives us green soon and we can call this one done!

[Lochlan]

@cdaringe Thank you for making these most recent changes, I really appreciate it. And I'm greatly looking forward to improving our build, it definitely needs some attention!

Since it looks like we've reached consensus on this PR I'm going to go ahead and merge this in.