Fix tests

[paulcpederson]

Ran in to several problems getting the tests to run properly, which I think were related to me using a newer version of Node (v5.4.1).

I've fixed those errors, and I think we should consider newer versions to the travis.yml to ensure that it passes in every Node environment.

/cc @cdaringe @Lochlan

[cdaringe]

i actually tried bumping the version in the travis.yml, however, the build then fails. needs some further sleuthing! otherwise, +1

[cdaringe]

i don't think that has to happen in this PR. I'll file an issue to help us update the build process

[paulcpederson]

@cdaringe ok, sounds good.

Looks like there was an error with the build, which is the difference between old and new versions. Just tested locally and if I use 0.10.x the test fails, but if I use 0.12+ the tests pass. I'm going to make that test less brittle so both versions pass. That will enable us to add more versions to travis.yml in the future and have them pass!

[paulcpederson]

👍

[paulcpederson]

@cdaringe ok, I'm testing to see if all Node versions pass in travis now. Essentially I added all LTS releases:

When stable changes, we'll just add the LTS version to the list as well.

Doing that made the tests fail, as you mentioned earlier. It seems to be a problem with this bug: tav/nodelint#40

nodelint seems pretty stale. Hasn't been updated substantially in like 4 years. What do you think about moving to something like https://www.npmjs.com/package/jslint ? Or anything else? I think there would be some pretty substantial updates to the codebase in terms of coding style that would need to happen in order to make this happen, so wanted to get your feedback before I just started updating the entire codebase.

[paulcpederson]

@cdaringe ok, sorry I kept tacking on to this pr. Essentially does the following:

1. Update tests that were breaking in newer versions of Node
2. Change from nodelint to jslint
3. Small changes to lib and test files to get jslint to pass
4. Adds all LTS Node versions to .travis.yml
5. Updates the pre commit hook to parse new jslint output
6. compile + run test now cleans up tmp folder after running

[Lochlan]

Thanks for fixing tests!

I agree that getting off nodelint seems good, but...jslint? Isn't jshint the better/more mature linter here? I would vote to use that (and a .jshintrc file) instead of jslint.

[Lochlan]

Is this to satisfy the linter? IMO this doesn't add clarity, it just seems crufty.

[paulcpederson]

yes, every instance in which I'm returning inside an empty function is to satisfy the linter.

[Lochlan]

Other than my complaints about the linting product being chosen here and the return statements, this PR looks good to me.

[paulcpederson]

@Lochlan I actually prefer jshint to jslint as well. Maybe @cdaringe has opinions?

[cdaringe]

jshint, from my experience, doesn't offer very great auto-correction tools. i'm ok with any improvement by this point, however! my bias is first https://github.com/feross/standard, then eslint, and finally jshint. the prior two offer a --fix or --format flag to help out. if you guys are both pro- jshint, however, sounds ok to me :)

[cdaringe]

👍 💯

[cdaringe]

BTW, i think tracking with LTS is the ideal plan!

[paulcpederson]

Moving to standard would be a big update but I am super down to do it. I've been using it for pretty much every project lately 👍

[Lochlan]

Personally I am not a huge fan of feross/standard's stylistic choices (re: semicolons, I think I like almost every other opinion it has), but I can appreciate the aims of the project. Lines like ;[1, 2, 3].forEach(bar) are unnatural for me. And since this would be, as @paulcpederson points out, "a big update," there are also some slightly annoying git blame consequences. I am, however, totally fine with eslint instead of jshint.

i'm ok with any improvement by this point, however!

I agree strongly on this point, and I don't necessarily think the linter choice should hold up merging this PR.

[paulcpederson]

@Lochlan agree. I think jslint will do for now, as the project was already using jslint so it really only updated the cli aspect.

Let's just open an issue about choosing a new linter and we can get this pr out the door so that people using Node versions 4+ can install the project.

[cdaringe]

we ready to merge? 💪 +1 from me! over in ampersand, we do two +1s on a PR to get it in (from someone who didnt do the PR), and none for documentation changes. OK with that?

[Lochlan]

It seems like we've reached consensus so I'm proceeding with the merge.

I am totally on board with the "two +1s" rule. The no +1s on documentation seems ok as well, I have never tried that approach but I am open to it.