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

group errors by file #409

Open
charlierudolph opened this issue May 4, 2017 · 9 comments
Open

group errors by file #409

charlierudolph opened this issue May 4, 2017 · 9 comments

Comments

@charlierudolph
Copy link
Contributor

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
Copy link
Owner

SimenB commented May 5, 2017

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.

@SimenB SimenB closed this as completed May 5, 2017
@SimenB SimenB added the question label May 5, 2017
@charlierudolph
Copy link
Contributor Author

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
Copy link
Owner

SimenB commented May 5, 2017

I agree

@PalleZingmark
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Collaborator

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
Copy link
Contributor

PalleZingmark commented Oct 27, 2017

@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
Copy link
Contributor

PalleZingmark commented Oct 28, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants