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
refactor: Break out code into testable modules #265
Conversation
100% unit test coverage for broken-out code
The latest commit breaks out It also adds logging infrastructure throughout the language server to allow for less time spent figuring out why something doesn't work and more spent fixing what needs fixing. Winston is used as a logger and a custom transport attaches it to the language server connection so that it may emit logs through the remote console, formatted using a custom formatter. Most code that isn't in I pushed what I have up now because I do not want to lose all the hours of work I've been putting into this in the event my computer decides to take a dump. The video card is on its way out, or at least I think that's what it is, so I'm playing it safe. |
There's a phenomenal amount of work here! Thank you! Do you need more time to prepare for version 1? |
A day or two at the most, I think. Testability is really important for me for v1 because 14 is a big release and I want it to be easy to make sure we don't break things when we change things and when we inevitably get bug reports for the extension. I've been working on this extension essentially with the hours of a full-time job on weekdays for close to a month now, and I even spent this weekend working on this refactor. And yet it's still taking a tad longer than I thought it would, though not by much! Most of what's left is writing test suites, I have another three written since I pushed up the last commit. The biggest challenge was breaking up the server implementation, and that's done. |
That's fantastic!
You've put a remarkable amount of effort into this. Open source work is often thankless, so I'll reiterate my appreciation of your efforts. Thank you!
I'll pop by on Wednesday to see how it's going. In the meantime, I'll prepare the stylelint repo for release and give all the other related pull requests another look over (7 other repos will need to be released/updated in sequence!). |
What @jeddy3 said, amazing amount of work @adalinesimonian, thank you so much 🙇🏼♂️💯 I've been running the v1 of this extension for a week locally, using it every day, haven't noticed anything untoward thus far. Happy to update my extension locally when this PR code is ready, just let me know when it's a good time to update my v1 of the extension of compile a new version from this branch @adalinesimonian |
Entry points excluded, this PR now brings all code to 100% statement, branch, function, and line coverage in unit tests. That doesn't mean all cases are tested, however, and end-to-end tests definitely need work. But that can take place in a separate PR and does not need to block v1. Tests have been renamed to unit, integration, and e2e (end-to-end), which better reflects what they actually are. A lint script has been added that requires any new non-entry point modules to have a corresponding unit test file. A readme has been added to the |
Unmarked as draft. Looking forward to your reviews! @ntwb Thank you for giving v1 the real-life test it needs! We really don't have enough e2e tests and your help is incredibly valuable! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An incredible amount of work. Thank you so much!
Reading through the code was a learning experience for me! This codebase and VS Code extensions in general are both unfamiliar to me, but this refactor has made it much clearer what all the moving parts are and what they do. The additional logging and diagnostics look great too, and will help in the debugging and fixing of issues!
I can say that the invocation of Stylelint, the setting of options etc, looks good to me.
DRAFT TestingNote: Sharing this initial feedback now, will update and continue to make further notes Summary: It's midnight here, and I've run out of steam, cli is working, but not VS Code right now My ConfigA preview of Stylelint v14 is published as To install I've updated my VS Code settings per https://github.com/stylelint/vscode-stylelint/tree/v1#migrating-from-vscode-stylelint-0xstylelint-13x
"css.validate": false,
"less.validate": false,
"scss.validate": false,
"stylelint.validate": ["css", "scss"]
"lint:vss": "stylelint '**/*scss'", Testing✅ CLITesting is working fine in my terminal with both npx stylelint "**/*.{css,scss}"
src/scss/components/_burger-nav.scss
349:22 ✖ Unexpected whitespace after "(" media-feature-parentheses-space-inside
349:39 ✖ Unexpected whitespace before ")" media-feature-parentheses-space-inside
src/scss/components/_footer.scss
337:4 ✖ Unexpected empty line before closing brace block-closing-brace-empty-line-before
src/scss/components/_masthead-secondary.scss
52:2 ✖ Unexpected empty line before closing brace block-closing-brace-empty-line-before
src/scss/components/_masthead.scss
40:11 ✖ Unexpected whitespace after "(" media-feature-parentheses-space-inside
40:40 ✖ Unexpected whitespace before ")" media-feature-parentheses-space-inside
src/scss/components/_post-card.scss
114:2 ✖ Unexpected empty line before closing brace block-closing-brace-empty-line-before
214:10 ✖ Unexpected whitespace after "(" media-feature-parentheses-space-inside
214:39 ✖ Unexpected whitespace before ")" media-feature-parentheses-space-inside
317:10 ✖ Unexpected whitespace after "(" media-feature-parentheses-space-inside
317:39 ✖ Unexpected whitespace before ")" media-feature-parentheses-space-inside
src/scss/utilities/_custom-properties.scss
67:2 ✖ Unexpected empty line before custom property custom-property-empty-line-before
src/scss/utilities/_ie11-toolkit.scss
37:4 ✖ Unexpected whitespace after "(" media-feature-parentheses-space-inside
37:28 ✖ Unexpected whitespace before ")" media-feature-parentheses-space-inside
38:4 ✖ Unexpected whitespace after "(" media-feature-parentheses-space-inside
38:30 ✖ Unexpected whitespace before ")" media-feature-parentheses-space-inside
src/scss/utilities/_mixins-general.scss
38:21 ✖ Unexpected whitespace after "(" media-feature-parentheses-space-inside
38:36 ✖ Unexpected whitespace before ")" media-feature-parentheses-space-inside
44:4 ✖ Expected newline after "}" block-closing-brace-newline-after
44:5 ✖ Expected empty line before at-rule at-rule-empty-line-before
src/scss/components/archive/_article.scss
42:2 ✖ Unexpected empty line before closing brace block-closing-brace-empty-line-before
src/scss/components/homepage/_topics.scss
22:2 ✖ Unexpected empty line before closing brace block-closing-brace-empty-line-before
464:1 ✖ Unexpected empty line before closing brace block-closing-brace-empty-line-before
src/scss/components/homepage/_video-carousel.scss
124:1 ✖ Unexpected empty line before closing brace block-closing-brace-empty-line-before
src/scss/components/post/_content.scss
102:1 ✖ Unexpected empty line before closing brace block-closing-brace-empty-line-before
|
Same as eslint
Logging as an error is too severe a level.
@ntwb Confirmed that untitled documents weren't getting lint suggestions. The issue was that they technically don't belong to any workspace, so I've downgraded the resolution failure log to a warning instead of an error since it's really more of a warning anyway. I'm not sure why you're not getting any resolutions, however, I will take a look in a second and see if I can replicate this behaviour. |
FYI, if you need to do any manual testing all the packages have been prereleased under the |
Sick! That's awesome; I'll update the dependency to use |
using stylelint@next
@ntwb I created a new folder, installed Stylelint globally, and the extension was able to resolve and lint using Stylelint in that workspace. Then I removed the global installation and installed Stylelint locally, and that also worked. I'm not sure how to replicate the behaviour you're seeing; any details I might need to know? |
Thanks, I cannot think of anything that may be unique to my circumstances. As such, I created a new test repo based of a local test repo I've had for some time: To test, checkout the repo and I've added a VS Code workspace file and settings in I've setup VS Code Insiders for this, the only extension I have installed is the latest from this branch with the changes you've made since my last comment I have not configured any custom syntaxes whilst there are additional files in the repo I'm only trying to get this working right now with only CSS and I see the same issues as I reported above
|
Perfect, thanks! This will undoubtedly help. I'll pull it down and take a look in a sec! |
@ntwb Very strange! I pulled down the repository, ran I added the correct configuration for SCSS to see if it'd lint, and that worked, too: Shooting in the dark here, are you maybe running a platform other than Windows? I can grab my MacBook and see if that changes anything. |
I'm using MacOS, but, I'm on an M1 ARM64 Mac,let me pull this repo down on my Intel Macbook Same results, installed VS Code Insiders, cloned the repo, The console shows the following, which is the same on M1 & Intel:
|
Weird! I just did the same on my Mac, it's an Intel, seemed to work: Could you run VS Code with |
I'm going to have to start the releases in an hour or so as I'm now unlikely to have time to do it any other time in the coming weeks. Hopefully that doesn't throw a spanner into the works here, sorry! |
That should be fine! I'm just waiting to hear from @ntwb. It shouldn't take much to cut a release here. If you have the time to, it'd be very helpful if you could install this version of the extension and see if linting works for you. I can provide a vsix if you'd like, you'd just have to run @jeddy3 One thing I was thinking of, would it be helpful to display a one-time warning in any workspaces with Stylelint 13 or lower that it is no longer supported? It doesn't have to block the extension's release, we could push it out in a patch. |
That'd be great. I'll give it a go if you point me in the direction of the
That's a good shout.
I agree. I'm about an hour into the release process (dotting the i's and crossing the t's at the moment). I'll be checking off the items in the release sequence as I go. You can follow along there if you want to know where I'm at with it. |
@jeddy3 Here's a ZIP with the VSIX inside. |
@adalinesimonian Working at my end. Intel Mac. Correctly reporting errors in my local test suite with stylelint@14. |
Wooo! That's good to hear. Time in aus is 2:50 AM, so it may be a bit before we hear from @ntwb. Any issues with delaying the v1 release until then? Stylelint should continue to work with 0.x in the meantime, I just tested it to make sure it would. |
SGTM. Once it's out, we can rustle up a tweet on the Stylelint channel.
I verified that too before I started the release process :) |
I've finished the release process and I'm going to call it a day 🎉 Good luck with your release! |
src, scripts, and tests use separate configs now
Apologies, I tried with Zip version you added @adalinesimonian, same results, but it's working for everyone else, so yolo, go for it, will keep an eye on things and do some more testing later tonight (~8 hours from now), but feel free to merge and release 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank YOU @adalinesimonian, astonishing amazing awesome work 🚀
No problem! I'm really curious why it's not working for you. Do share whatever logs you get using the development environment in a new issue if you continue to have problems. I have to say it's a little worrying that something might be wrong but I also can't wait to release this bad boy! Guess it's release time, y'all! Thank you all again for all your help and input on this! |
@adalinesimonian Fantastic stuff! Thanks again for all your hard work overhauling this extension. Your timing was impeccable too as this extension was unmaintained leading into the release of 14.0.0. It all came together and now we have a fresh CSS linter and a fresh VS Code extension to go with it. Thank you for making that happen! |
Closes #255
Architectural Overview