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

CSS lint support for arcanist using csslint. #93

Merged
merged 3 commits into from Jul 22, 2013
Merged

Conversation

@vlajos
Copy link
Contributor

@vlajos vlajos commented Jul 9, 2013

CSS lint support for arcanist using csslint.

@vlajos
Copy link
Contributor Author

@vlajos vlajos commented Jul 22, 2013

ping.

@epriestley
Copy link
Member

@epriestley epriestley commented Jul 22, 2013

I'm a bit hesitant to merge any more external linters until after https://secure.phabricator.com/T2039

The problem is that external linters create a support problem which I don't yet have enough infrastructure to deal with, and adding more tends to exacerbate this problem. There's a small war being waged over one of the Python linters now, where the correct python binary is different on every system. One solution is "add lots of configuration options to every linter", but I think T2039 is preferable.

This code looks correct to me -- how about I try to get T2039 done by Aug 1, and I'll merge this as-is if I can't get it done by then?

autocracy added a commit that referenced this pull request Jul 22, 2013
CSS lint support for arcanist using csslint.
@autocracy autocracy merged commit 2852a96 into phacility:master Jul 22, 2013
@epriestley
Copy link
Member

@epriestley epriestley commented Jul 22, 2013

@autocracy, I don't think we've met -- are you a Facebook employee? Can you explain why you merged this? I'm on #phabricator on FreeNode if you want to chat.

@autocracy
Copy link
Contributor

@autocracy autocracy commented Jul 22, 2013

Read code, merge file, check email, oops.

Github doesn't have an unmerge, does it?

On Jul 22, 2013, at 6:44 AM, Evan Priestley wrote:

I'm a bit hesitant to merge any more external linters until after https://secure.phabricator.com/T2039

The problem is that external linters create a support problem which I don't yet have enough infrastructure to deal with, and adding more tends to exacerbate this problem. There's a small war being waged over one of the Python linters now, where the correct python binary is different on every system. One solution is "add lots of configuration options to every linter", but I think T2039 is preferable.

This code looks correct to me -- how about I try to get T2039 done by Aug 1, and I'll merge this as-is if I can't get it done by then?


Reply to this email directly or view it on GitHub.

@epriestley
Copy link
Member

@epriestley epriestley commented Jul 22, 2013

I'm going to revert this as per #93 (comment)

@vlajos
Copy link
Contributor Author

@vlajos vlajos commented Jul 22, 2013

Let me know if I can help something.

epriestley pushed a commit that referenced this pull request Jul 22, 2013
This reverts commit 2852a96, reversing
changes made to 46bb3db.

See: #93
@epriestley
Copy link
Member

@epriestley epriestley commented Aug 2, 2013

It's Aug 2 and I haven't made any progress on generalizing lint, so I'll pull this.

@epriestley
Copy link
Member

@epriestley epriestley commented Aug 2, 2013

I'm dropping this from the Comprehensive lint engine in the pull because adding it there turns it on for everyone using that engine with no way to disable it, and that engine is seeing far more use than it was intended to (this is among the reasons that the current approach is bad), and if you don't have csslint installed adding it makes the engine fail. Everything else is going through. Thanks!

@epriestley
Copy link
Member

@epriestley epriestley commented Aug 2, 2013

Pulled as a680c55

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