SimenB / stylint

Improve your Stylus code with Stylint
https://simenb.github.io/stylint/
GNU General Public License v2.0
348 stars 62 forks source link

group errors by file #409

Open charlierudolph opened 7 years ago

charlierudolph commented 7 years ago

Currently you get an output like:

path/to/file1.styl
29 sortOrder error prefer alphabetical when sorting properties

path/to/file1.styl
33 sortOrder error prefer alphabetical when sorting properties

path/to/file1.styl
39:13 colons error unnecessary colon found

path/to/file1.styl
43 sortOrder error prefer alphabetical when sorting properties

path/to/file1.styl
44 sortOrder error prefer alphabetical when sorting properties

path/to/file1.styl
48 sortOrder error prefer alphabetical when sorting properties

while I would prefer output that looks like

path/to/file1.styl
29 sortOrder error prefer alphabetical when sorting properties
33 sortOrder error prefer alphabetical when sorting properties
39:13 colons error unnecessary colon found
43 sortOrder error prefer alphabetical when sorting properties
44 sortOrder error prefer alphabetical when sorting properties
48 sortOrder error prefer alphabetical when sorting properties
SimenB commented 7 years ago

This is possible today. https://github.com/SimenB/stylint/blob/master/README.md#groupoutputbyfile--default-true-true--false-

Ping me if it doesn't work, and I'll reopen.

charlierudolph commented 7 years ago

Oh thanks. Good to know.

One unexpected part of this is the following: I have a .stylintrc file that does not have the key groupOutputByFile, yet it is not defaulted to true. In my experience with other tools, if I don't specify a key, then it is given its default value. That would be preferable in the case.

SimenB commented 7 years ago

I agree

PalleZingmark commented 6 years ago

As far as I can see, this could be fixed by returning true on Line 27 in src/core/done.js if this.config.groupOutputByFile doesn't exist.

var group = this.config.groupOutputByFile || true 

I have a PR ready for this if agreed.

charlierudolph commented 6 years ago

@PalleZingmark I don't think that would work. If the person set it to false, that would override it to make it true. We only want to default it to true if the groupOutputByFile key is not present in the supplied config.

PalleZingmark commented 6 years ago

@charlierudolph Lol, of course, those damn false javascript false being false. ;)

So I guess checking that the key actually exists would be a better way, e.g.

var group = ('groupOutputByFile' in this.config) ? this.config.groupOutputByFile : true 
wojciechczerniak commented 6 years ago

There is a default config already: https://github.com/SimenB/stylint/blob/master/src/core/config.js

Don't create another defaults in more places. This will be unmaintainable in the future.

The correct flow is to merge config files from various sources and use final value everywhere. Lets find why this fail first.

PalleZingmark commented 6 years ago

@wojciechczerniak True, good point.

As far as I can see the config used is set in src/utils/setConfig.js based on X criterias, but as the description says:

@description overrides default config with a new config object

It only overrides the default config object with a new config object, hence a check for non-existent props in that config will return false and this problem will occur.

PalleZingmark commented 6 years ago

Changed my PR to use this approach instead of doing a check to whether the value exists or not. setConfig will now return a complete config based on src/core/config.js, but with updated properties if provided.

Need to update tests.

Update: Tests are in