search

node-swig / swig-templates

Take a swig of the best template engine for JavaScript.
http://node-swig.github.io/swig-templates/
MIT License
209 stars 29 forks source link

Fix tests #9

Closed paulcpederson closed 8 years ago

paulcpederson commented 8 years ago

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 commented 8 years ago

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

cdaringe commented 8 years ago

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

paulcpederson commented 8 years ago

@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 commented 8 years ago

:+1:

paulcpederson commented 8 years ago

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

schedule

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: https://github.com/tav/nodelint/issues/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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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

paulcpederson commented 8 years ago

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

cdaringe commented 8 years ago

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 commented 8 years ago

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

paulcpederson commented 8 years ago

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 :+1:

Lochlan commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

we ready to merge? :muscle: +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 commented 8 years ago

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.