kungfusheep / SublimeLinter-contrib-stylelint

this repo is no longer maintained - please see https://github.com/SublimeLinter/SublimeLinter-stylelint
MIT License
116 stars 19 forks source link

Use NodeLinter to support local stylelint #19

Closed danielkcz closed 8 years ago

danielkcz commented 8 years ago

I am not really a Python developer, all credit goes to @daniele-rapagnani with this PR for coffeelint.

I tried this by myself and it works quite nicely indeed. There is no need for stylelint_wrapper.js anymore apparently. Perhaps there are some other cases I am missing here so I don't expect merge right away, but it's a start...

danielkcz commented 8 years ago

I am not sure about failing test, that's completely black area for me...

jahvi commented 8 years ago

Can confirm @FredyC PR fixes the "postcss not installed problem", the test fails because of the following issues:

./linter.py:12:1: F401 'os' imported but unused ./linter.py:16:1: E302 expected 2 blank lines, found 1 ./linter.py:41:23: W292 no newline at end of file

Just remove the import os line, add a new blank line before the class declaration and a new line at the end of the file to fix them.

danielkcz commented 8 years ago

Thanks @jahvi, I missed those lines in test output actually. It's passing now, although I made small mistake and created another two commits. @kungfusheep If you want, I can rebase this and squash.

kungfusheep commented 8 years ago

Apologies - I haven't had any time for this plugin recently. I'm afraid the problem with your proposed change is that it breaks earlier versions of stylelint that didn't have the CLI, which is why the wrapper is still necessary.

I think some investigation into why the code in the wrapper is falling into the block where the postcss module is required - as this is the fallback to an old version when it can't detect the CLI. If nobody else can do this investigation then I should hopefully be able to do it within the next couple of weeks - cheers.

BTW - thanks for contributing.

danielkcz commented 8 years ago

Well I respect your decision, but personally I think it's a wrong one. Why don't you just release new version with breaking change and support only newer stylelint?

ntwb commented 8 years ago

The stylelint cli was added in v2.0.0, the current release is 5.3.0, that is 3 major versions since.

I think enough versions have passed with breaking changes, fixes, and general improvements to stylelint as a whole that still supporting 2.0.0 is really no longer warranted.

kungfusheep commented 8 years ago

Hey

I still know of teams, mine included, using older versions because updating would mean updating a lot of custom rules.

I'd be inclined to agree were it a significant amount of time since 2.0.0 was released, but it's just under 6 months ago, which isn't a long time at all really is it, if you consider projects that could've started before that time and still be in active development.

It's not always feasible to stay up to date with every node module that gets updated, especially if nothing is broken,so if it's not a great deal of effort to give people that leeway then I don't see that as a negative thing.

danielkcz commented 8 years ago

@kungfusheep Question is, do you actually plan to extend current version in some direction? If not, then your team (and others) can still use current version, but you can release breaking change version for others that want to move on. In my opinion it's less problematic and time consuming way than trying to investigate why this doesn't work.

jahvi commented 8 years ago

Is this still a problem in pre-cli versions? If not then people using stylint < 2.0.0 can stick to using the current version and the rest can update as needed.