Linting JS and Coffee test specs

[Merott]

At the moment, the linting process for JS files doesn't include .spec.js files. Shouldn't specs also be linted?

The same issue applies to CoffeeScript files and spell-coffeescript.

[joshdmiller]

Indeed they should... so we need to figure out how to do that without having the spec files copied as part of the build.

Option 1. Add linting to the karma spells. I don't think I like this because we're including linting in two places, so any customization to the linting has to go in two places.

Option 2. Include them in the build by default in webapp-spell (right now they're manually excluded), then have webapp-karma filter them out with a new stream function:

# re-open the flow and add a step directly after linting
warlock.flows "scripts-to-build"
.add 13, "filter-out-tests", warlock.streams.filterOut( "globs.source.test" )

I don't like this because it feels weird and kludgey.

Option 3. A variation on the above, where we "fork" the specs out of scripts-to-build. Instead of using a globbing pattern as the source, we specify the globs against another flow:

warlock.flow 'karma-single-run',
    fork: "scripts-to-build@13"
    fork_options:
      filter: [ "<%= globs.source.test %>" ]
      remove: true # to remove the forked values from the original flow
    tasks: [ 'webapp-build' ]

# all the added steps are the same...

I'm not sure how I feel about this one, but it does seem a little confusing.

Did I miss any?

[Merott]

I don't like 1 and 2 either. I can understand option 3, but I'm not sure how I feel about it. The spec files aren't really part of the build, so it feels weird for them to be included in scripts-to-build.

Couldn't we have a scripts-lint task, and then have scripts-to-build and karma-single-run to depend on it?

[joshdmiller]

The spec files aren't really part of the build...

Only if the Karma spec is included. I made a mistake when I created the webapp spell in that the globbing patterns assume the pattern tests will take in order to negate them. If one wanted to use *.test.js instead of *.spec.js, they would have to change the Karma spell and the globs for the webapp spell or their tests would get included; that's pretty dumb. It would be much better if the the webapp spell could be told to ignore the right files, in which case something like the above may work because the specs would be part of scripts-to-build by default and we would need to remove them somehow anyway.

Couldn't we have a scripts-lint task, and then have scripts-to-build and karma-single-run to depend on it?

Sure, but then we still have one or more of these problems, depending on the implementation: (a) the lint settings are still defined in multiple places; (b) spells contain globbing patterns that don't concern them; (c) globs are duplicated across spells; and/or (d) spells become interdependent. Unless there's some conception of this I've missed.

[Merott]

Only if the Karma spec is included.

You meant karma _spell_, got me confused for a sec.

You're right. The webapp spell should assume all *.js files are part of the build, unless another spell (e.g. karma spell) comes along and removes *.spec.js from the build.

[joshdmiller]

Oops. Indeed I did.

This is probably something that will take a bit of thought as I can see this being a recurring theme across many different spells.

[Merott]

How about this?

https://github.com/Merott/spell-webapp/commit/ef5d456c7612d09b87f268640e23f5c06c954bed https://github.com/Merott/spell-karma/commit/f67b2aa063dfd033fc189f3b3deb858185171ea8 https://github.com/Merott/spell-coffeescript/commit/c9fb888fcac73b82e7f3829645a06aa0ad29092e

I find this easier to comprehend than flows using fork and fork_options.remove to alter glob patterns on the fly (not technically alter, but practically)

It also doesn't require any change to the Warlock engine itself.

What do you think?