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

Add JSON formatter #642

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sergeychernyshev
Copy link
Contributor

JSON formatter for CSSLint results

@sergeychernyshev
Copy link
Contributor Author

Not sure if this implementation is correct, considering that formatResults() returns nothing and all work is done in endFormat().

This works, but feels bad and some users of CSSLint call formatResults directly.

Not sure how to do this better considering that output from multiple files is simply concatenated and wrapped with startFormat() and endFormat() - this doesn't work for JSON, obviously as it requires commas between results.

@Arcanemagus
Copy link
Contributor

Duplicate of #606, but I'd be happy if either of our PR's get accepted...

@Arcanemagus
Copy link
Contributor

This doesn't take into account the quiet option, but then mine doesn't take into account more than one file 😛.

@sergeychernyshev
Copy link
Contributor Author

@Arcanemagus I can try merging the two implementations.

@sergeychernyshev
Copy link
Contributor Author

Anyone has any idea why travis-ci build is failing? from the logs it doesn't seem like there are any issues with the code, just some permissions issues with the build script.

@Arcanemagus
Copy link
Contributor

Updated #606 with proper support for multiple files, mind looking over it and making sure I'm not missing anything?

@frvge
Copy link
Contributor

frvge commented Mar 6, 2016

Re-ran the tests now that it's Node 5.7.1

@sergeychernyshev
Copy link
Contributor Author

I looked at #606 and it turns out that both implementations are good on their own accord, but merging them together requires us to resolve issue #645 that I created.

I'll try to take a stab at it soon - will need to introduce new way of handling multi-result formatters.

@sergeychernyshev
Copy link
Contributor Author

@frvge thank you, I was wondering if it's my code breaking something I don't understand.

@frvge
Copy link
Contributor

frvge commented Apr 4, 2016

@sergeychernyshev , we've merged #606. Is your version still needed?

@sergeychernyshev
Copy link
Contributor Author

sergeychernyshev commented Apr 4, 2016

@frvge yes, it is still needed for multi-file support, but it'll have to be updated to merge with support that #606 provided. Should not be pulled as is.

Can you check issue #645 and comment on the issue?

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

Successfully merging this pull request may close these issues.

3 participants