Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cache rule name with error message for reporters to use as they need #264

Merged
merged 2 commits into from
Apr 1, 2016

Conversation

wojciechczerniak
Copy link
Collaborator

Just like ESLint and sass-lint expose rule name with error message cache. Not every reporter need to use it but will be useful to display it in editor like ESLint do. Possible future use: find automatic fix by rule name and apply it inline (alt+enter inspections in Webstorm IDE).

stylint-rule

eslint-rule

BTW. What's the difference between state and cache objects? Some warning message properties are on state (severity) and some are on cache (lineNo, msg, etc). Did I choose the right one?

@SimenB
Copy link
Owner

SimenB commented Mar 27, 2016

What's the difference between state and cache objects?

I don't know actually, they seem to overlap somewhat.

I think we either should make the distinction clear (state being stuff regarding current file, and cache being overall state regarding the whole run), or just merge them together. @rossPatton thoughts? Both are breaking changes, but if you agree, we could do some more breakage, like finally finishing off #161.
I also think it would be nice to only call the reporter after all files are processed, instead of after each single file, at least as an option.

@wojciechczerniak WRT this change, huge 👍 A test would be nice here, though.

And I'm really looking forward to the plugin! 😄

@rossPatton
Copy link
Collaborator

i'm cool with merging them, or really with whatever you guys want to do. everything kind of just grew organically, so i'm sure there's tons of room for improvement.

@SimenB
Copy link
Owner

SimenB commented Mar 27, 2016

Cool, made #269 to list any other potential breakage we might want.

@wojciechczerniak A test here, then I'm happy with this PR at least 😄

@wojciechczerniak
Copy link
Collaborator Author

I'm not sure if commit triggered notification so just letting you know about added test.

BTW. JetBrains plugin is released now: stylint-plugin

@SimenB
Copy link
Owner

SimenB commented Mar 30, 2016

LGTM. Unsure why the build is failing now, though... Rebase on master?

I'll test out the plugin asap! Really cool that you made it

@wojciechczerniak
Copy link
Collaborator Author

My test fails on node v4 and below in getFiles.js. I'll investigate.

@wojciechczerniak wojciechczerniak force-pushed the cache-rule-name-with-warning-message branch from 033b3f4 to e9eccb6 Compare April 1, 2016 06:49
@wojciechczerniak
Copy link
Collaborator Author

No problem here. Rebase was the right way to go, thanks.

@SimenB SimenB merged commit 7911741 into SimenB:develop Apr 1, 2016
@SimenB
Copy link
Owner

SimenB commented Apr 1, 2016

@wojciechczerniak now it failed only on node 4... Something weird is going on

@wojciechczerniak
Copy link
Collaborator Author

My mistake was to change node version only. I've reproduced Travis step by step. Deleting node_modules each time and npm install && npm test caused the glob() in getFiles.js to throw exception.

With node_modules from v.5 tests are green (?).

When node version is changed the npm changes too. In v5 modules are "flat", in lower versions glob module has it's own node_modules.

@SimenB
Copy link
Owner

SimenB commented Apr 1, 2016

run npm dedupe on install then? Seems buggy though...

@wojciechczerniak
Copy link
Collaborator Author

It's weird. But probably not for us to fix. We deal with async call here which is messed up by configuration change. IMO this exposed problem with this.config writes which I address in #272

@wojciechczerniak wojciechczerniak deleted the cache-rule-name-with-warning-message branch April 9, 2016 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants