Proposed changes for 2.0 release

[SimenB]

Mostly to kick of the convo, and try to centralise it, I'll try to compose a list of potential breaking changes for a 2.0 release.

I'll just throw together a quick list now, can probably be expanded quite easily 😄

• Merge this.cache and this.config (cache rule name with error message for reporters to use as they need #264 (comment))
• Instead of caching a formatted string in this.cache.warnings & this.cache.errs, have an object with some metadata (such as if it's a warning or not)
  • Then merge this.cache.warnings & this.cache.errs
  • This is done in Lint string #274
• Potentially not call reporters until all files are processed, and just build up some state, like http://eslint.org/docs/developer-guide/nodejs-api#executeonfiles
  • I also think we should call reporters with arguments, instead of making them rely on this.state/config. That makes them easier to write I think, as the interface is more clearly defined.
• Somehow make each part a bit more unit testable.
  • Example is calling init should not call read, as that makes testing init in isolation a pain. Might not be a breaking change, not sure. Also unsure if that works with stampit?

/cc @rossPatton @wojciechczerniak

[wojciechczerniak]

I agree with all the points.

It would be a lot easier to unit test rules/checks if they returned an object with message, column, severity, result or null if not found. Then all those results could be added to single this.cache.warning/error array and passed to reporter at the end as you proposed. Maybe checks should also get settings as param, and don't read this.cache directly. We could simplify cache reset etc. After those changes this.cache and this.state would be the same, storing state of the stylint, obvious merge.

Also each check is called once per line, see #266. Maybe we could call each rule again and again until null is reported which means no more errors of this type found.

Automatic fixes would be a nice addition #236. Probalby not a breaking change and could be done as feature, but it's good to keep it in mind.

Don't forget #161.

[SimenB]

I've created a v2 branch off of develop now, thought to keep the breakage in there until closer to release. I've rebase #274 off of develop, should be able to wrap it up soon ish, as breakage won't matter (much).

And yes, decoupling checks, along with reporters, from this would be great.

Perhaps providing a way for custom checks to be registered by the user as well?

[SimenB]

Thinking about it, how about passing in a single object to checks, like eslint does, containing different stuff, like the line, source, config and a reporter function? http://eslint.org/docs/developer-guide/working-with-rules#the-context-object

We don't have AST, but the current regex solution works fine, so no need to return anything. For testing purposes, just passing a mock as context.report or whatever should be no problem.

IMO, Stampit works fine within a project, but when exposing extension points for other modules, it requires too much domain knowledge. A pure function of input is easier to reason about than the internals of this.state, this.cache, this.config etc.. So if all checks receive a reporter function they call with an object (or multiple) violations they find, that's all they do, and all they need to know. And the complete array of violations are passed to reporters after all files are done.

Thoughts?

[gscottolson]

@SimenB: Perhaps providing a way for custom checks to be registered by the user as well?

👍

[SimenB]

@wojciechczerniak I'm going to be trying to push towards this now. Are you around to help with this?
I'll try to spend some time this weekend at least (just moved into a new apartment, might be optimistic)

[wojciechczerniak]

I was away from Github recently but I'll try to help.

If were going for breaking changes I would add another one:

• Separate stylint config from rules, like eslint does. Group all rules configuration under rules 🎉 node and reporter, maxErrors, maxWarnings etc. at root node.

  reporter: 'stylint-stylish',
  maxErrors: 4,
  exclude: [],
  rules: {
    brackets: ...
  }

• Consolidate rule configuration under the rule. For example: namingConventionStrict is used by namingConvention so right now rule parses whole config instead of getting just options object i.e:

namingConvention: {
  expect: true,
  strict: true
}

• Same for reporter options

reporter: {
  name: 'default',
  options: {
    "columns": ["lineData", "severity", "description", "rule"],
    "columnSplitter": "  ",
    "showHeaders": false,
    "truncate": true
  }
}

But I agree that testing rules should be our priority. Split core and rules test, maybe even rules into separate files. So it's easier for us to work on them.

[wojciechczerniak]

I think that proposed above config change would allow for easier rules testing. This way we could call rule with line and ruleOptions as arguments and just compare returned result with expected ones. Right now we have to set setting and cache object properties before calling the rule. I'm not sure if we've got separation between tests.

Instead of:

  app.state.conf = 'single'
  assert.ok( quoteTest( '', '$var = "test string" ' ) )

Just call:

  assert.ok( quoteTest( '', '$var = "test string" ', 'single' ) ) // or options object

Also asserting true/false on function result is not the final output of rule check because it calls reporter this.msg() inside and both outputs can differ. There is huge place for an error where we test value returned by function but something else is used as final output. Either we return result object for this.msg() and rule never call it by itself or we need to listen with sinon for this.msg() calls and check arguments it was called with. Right now tests can pass and reporter is called with any other output anyway. What do you think?

[SimenB]

That sounds good to me!

I'd really like for this to disappear personally, but unsure if it's worth the effort.

But rules and reporters should definitely not care about it, they should receive stuff as arguments, and return a result.
Whether or not internal state beyond that is something on this is just that, internal.

[wojciechczerniak]

Cool. So we need a plan.

Refactoring rules so they are pure arguments and return object will touch every file and all test have to be rewritten then (no longer asserts are true/false). Let's do this as the last one and have everything else in place.

I will try to put them in order step by step:

• Split test for core and rules. Separate tests for core from tests for rules #351
• Config file refactoring: Config file structure changes #354
  • Move reporterOptions under reporter node.
  • Move all rules under rules node as eslint does.
  • Consolidate rule settings that are used together i.e. namingConventionStrict and namingConvention.
• Merge this.cache and this.state. Merge this.cache and this.state #355
• Rules refactoring: Rules shouldn't rely on this #356
  • They shouldn't call this.msg() internally.
  • They shouldn't use this internally, get all data passed as arguments.
  • Return result object: { line: 0, column: 0, severity: "warning", message: "Colon missing." }
  • Update all tests.
• Add errors to single this.cache.warning/error array (returned object already has a severity property) and call reporter only at the end of linting. Merge this.errs and this.warnings #357 Decouple reporter from this #358
• Each check is called once per line. Should find all errors within a line. Only first occurrence of warning per line is reported #266

Lets split those into smaller tasks under 2.0 milestone. Am I missing something?

I don't know where to put those two:

• Some API changes in Lint string #274, Lint a string #161 not sure if I understood those correctly.
• Somehow make each part a bit more unit testable

Also there are 18 bugs opened that need to be addressed in both versions. And 20 enhancement propositions we can assign to milestones or add help wanted label if its not essential and/or mark them with labels new rule / core. New rules are rather safe and implemented as modules are quick way for anyone who wants to contribute.

To deliver 2.0 sooner than later I think those tasks should be considered as future enhancements:

• Provide a way for custom checks to be registered by the user. (v2.x ?) Allow registering custom rules/checks #360
• Automatic fixes would be a nice addition. Formatter mode? #236 (v2.x ?)
• Validate config file. Options schemas #281 (v2.x ?)
• Use AST for fun. Can we use an ast to extend stylint? #303 (v3.0 ?)

[SimenB]

Thanks for structuring it! Please do create issues and tag them with the milestone 😄

A few comments:

Return result object: { line: 0, column: 0, severity: "warning", message: "Colon missing." }

A rule shouldn't care about severity, we know that at call time.

Some API changes in #274, #161 not sure if I understood those correctly.

If we think fresh, I think making linting a string work should be very easy. Most of the breakage there (IIRC) was to return different stuff, and not call done etc. So it should solve itself if we do the other parts.

To deliver 2.0 sooner than later I think those tasks should be considered as future enhancements

Yeah I agree.

[SimenB]

I created a Project for it (https://github.com/SimenB/stylint/projects/1), might be better than the milestone

I think I've opened up issues for most of this now, and added them to the project. Anything missing?

Make sure to pull stuff into in progress if you start work on it, and open PRs on the v2 branch.

I vote for rushing towards 2.0, and then fix bugs after we have a proper testing structure in place, and can easily make regression tests for every case we fix.

[SimenB]

Also, we should make sure to release alphas as early and often as possible. @wojciechczerniak what's your username on npm? I'll add you there so you are able to push release in case I'm unavailable for some reason 😄

[SimenB]

@danielhusar @tplusrex @awayken you are all collaborators here, are you available to contribute?

@iDev0urer @Arcanemagus would you be interested in helping out with this effort?

[SimenB]

Also pinging people maintaining packages using stylint: @xdissent @guerrero @vtfn @jackbrewer @spyl94 @limarc

[wojciechczerniak]

A rule shouldn't care about severity, we know that at call time.

You're right 👍

@wojciechczerniak what's your username on npm?

It's wojciechczerniak.

I vote for rushing towards 2.0, and then fix bugs after we have a proper testing structure in place, and can easily make regression tests for every case we fix.

Exactly my view. Again #356 is essential here.

Thanks for creating issues and updating the list. Wow, you're fast.

[SimenB]

Ok, added you as owner on npm as well. Please do start working on some things if you have the time 😄 If you'd be able to review my PR, that'd be a great help as well

[awayken]

Honestly, @SimenB, I haven't used Stylus or Stylint for ages. We've since moved our styling to other technologies. At this point, I don't see myself having the motivation or time to contribute to the project. Sorry!

[SimenB]

No problem 😄 I'll just remove you as collaborator then

[SimenB]

@wojciechczerniak Updated the readme, does it look OK to you?

993b5d0...ac54e7b

[wojciechczerniak]

@SimenB Very nice summary of all the goals we should aim for 👌

[rossPatton]

man, you guys are really cranking. this all looks great! i should have turned it over to you a long time ago