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

Change CSSLint to Stylelint #56

Closed
wants to merge 8 commits into from
Closed

Change CSSLint to Stylelint #56

wants to merge 8 commits into from

Conversation

rei-nert
Copy link

Change CSSLint as CSS lint to use Stylelint.
Solve #55 and #46
Test passed just like using CSSLint

Copy link
Owner

@turbio turbio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for contributing.

PR looks really good to me, I'll get it merged in soon as I get a chance to test it locally.

server/cssfile.js Show resolved Hide resolved
server/filemanager.js Outdated Show resolved Hide resolved
rei-nert and others added 2 commits April 14, 2021 22:17
@rei-nert
Copy link
Author

Changed stylelint promise to have a catch and fix a bug where parsed the CSS with lint errors, by making it a promise and using Promise.all() to have both fulfilled.

@rei-nert rei-nert requested a review from turbio April 20, 2021 14:21
@jdsutherland
Copy link
Contributor

jdsutherland commented May 3, 2021

I just checked out this PR and tried a few different files and they all errored after starting Bracey. Switching to Stylelint seems like it should resolve the many issues related to Bracey blowing up with files containing modern CSS features, so I hope this PR works out.

@rei-nert
Copy link
Author

rei-nert commented May 4, 2021

I just checked out this PR and tried a few different files and they all errored after starting Bracey. Switching to Stylelint seems like it should resolve the many issues related to Bracey blowing up with files containing modern CSS features, so I hope this PR works out.

Can you post the errors? All the tests passed, and I also checked with a few of my files and everything was working fine.
If you can post the log, I'd gladly try to solve the errors

@jdsutherland
Copy link
Contributor

jdsutherland commented May 5, 2021

Sorry, @rei-nert. I ran npm install forgetting that npm install --prefix server is required, so the problem was that stylelint was missing. I tried this again and it's working great. This should resolve #36.

Copy link
Owner

@turbio turbio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apologies for the long review time, finally got a chance to take a look. I think we should focus on simplifying the code. There are some particularly spooky behavior where tests should be failing but are instead passing due to a quirk of the test suite.

server/test/fixtures/test_css.css Outdated Show resolved Hide resolved
server/test/cssfile.js Outdated Show resolved Hide resolved
server/test/cssfile.js Outdated Show resolved Hide resolved
server/cssfile.js Outdated Show resolved Hide resolved
server/cssfile.js Outdated Show resolved Hide resolved
server/cssfile.js Show resolved Hide resolved
server/cssfile.js Outdated Show resolved Hide resolved
server/cssfile.js Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unclear exactly how/when this file is read by the library. It'd be better if this could be embedded into the code to avoid spooky action at a distance

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking in the Stylelint documentation, there is:

stylelint uses cosmiconfig to find and load your configuration object. Starting from the current working directory, it looks for the following possible sources:

  • a stylelint property in package.json
  • a .stylelintrc file
  • a stylelint.config.js file exporting a JS object
  • a stylelint.config.cjs file exporting a JS object. When running stylelint in JavaScript packages that specify "type":"module" in their package.json

No rules are turned on by default and there are no default values. You must explicitly configure each rule to turn it on.

You can extend an existing configuration (whether your own or a third-party one).

Popular configurations include:

stylelint-config-recommended - turns on just possible error rules
stylelint-config-standard - extends recommended one by turning on 60 stylistic rules

I used the standard configuration because I don't have much time to create a file with all the rules, and it'd be better to use the library standard config file

@rei-nert
Copy link
Author

Undo stylistic changes and undo promise around CSS parser, returning to previous behavior.
Also, I found that stylelint has a plugin to css parser and I'm trying to make it work

@rei-nert rei-nert closed this by deleting the head repository Sep 12, 2022
@noahgitsham
Copy link

Why was this never merged?
Could this project be adopted/forked by someone else if it is abandoned?

I'm considering writing a clone in lua for neovim.

@turbio
Copy link
Owner

turbio commented Jul 7, 2023

heyo @noahgitsham! IIRC this PR never got to state where it addressed all the comments and replicated existing behavior closely enough. Haven't really found the time to give this plugin the love it deserves and fix a lot of the outstanding bugs. Plus the codes a bit kludgey and architectures absolutely bizarre (originally wrote this in highschool 🙃). I'm super open to supporting any work around this plugin (e.g. add you as a collaborator or direct to a fork/clone) so lemme know (shoot me an email or hmu on discord @ turbio).

@noahgitsham
Copy link

@turbio Hey, it's lovely to hear that you haven't completely abandoned ship. This plugin is very impressive considering you wrote it in high school!
Admittedly I have written very little vimscript to be of any use to this project. While I like the idea of support for both vim and neovim, I'm not sure about the direction that Bram is taking vim in. I would personally much prefer to write in lua if I were to develop a plugin. I'll have a look through your code, but it's your decision if you wish to restart maintenance on this project.

@turbio
Copy link
Owner

turbio commented Jul 7, 2023

Thanks @noahgitsham. Big agree on a pure lua rewrite, I've flirted with the idea before but haven't found the motivation. The architecture ended up this way partly because of the insanity that is vimscript + vim's language bindings so I'm right there with you. Using python bindings + nodejs was by far the most regrettable decision and has led to endless problems distributing and developing this plugin. Everything inside neovim without all the moving parts sounds awesome.

I've no plans to maintain this repo in its current state beyond reviewing pull requests unless I find the motivation for a rewrite. If you have any technical questions let me know, always down to chat.

@noahgitsham
Copy link

@turbio cool, in that case I think I'll start making something and see where it goes. I've never worked on something like this, so I'll be learning as I go along. I'm imagining it will turn out as poorly written if not worse than your project 😅.

It's been great talking to you though, I'll almost certainly be in contact with any questions

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.

5 participants