-
-
Notifications
You must be signed in to change notification settings - Fork 143
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 Automated updated of autocomplete-css
completions.json
#398
Conversation
This is very impressive, I didn't get a chance to review all the JS of the updater script or anything like that, but the spirit of this seems spot on. Would love to take a closer look if I have time, but it seems pretty in-depth. Lite approve ✅, I don't mind this merging, just wish I could be more thorough in reviewing it at the moment! |
Appreciate it! Feel free to ask any questions about it that you are curious about. This one did end up being way more complicated than originally intended |
… than just a single specs `value`
So with all the changes in, and all of the first issues people noticed having been resolved. I've moved on to looking at the tests: Which are not what we are hoping for:
So obviously we have many failures within the tests of |
Some of it, IIRC from trying similar a while ago, was a hard-coded expected number of properties or values or some such assertion. That'll need updating. Perhaps various other checks assume old values/properties. |
Yeah, it's a little strange. Essentially the vast majority that are failing are expecting the top level completions that would be returned. For example Which honestly checks out. This makes sense that you would expect this rather than, what we are currently getting, But honestly as I don't know coffeescript, or the existing code here, I'm hoping the change could be simple. Possible as simple as ordering the completions before being passed back to the user manually, such as I suggested on the linked issue in my PR description using LCS to rank the results then return them. Although I'd honestly much rather see a change like that land in |
Not sure exactly what the "ordered completions" file was about, that they went through the trouble of referring to usage statistics and place more popularly used properties/values first, based on crawling the actual websites of the web and inspecting the properties/values used most. This PR deletes those files, so I was going to assume they had only been used for generating the list of completions, and yet the ordering bit feels like it would be unnecessary in that case... So I'm not sure where, but I get a feeling the "ordered completions" file(s) may still be needed... ?? Or some ordering they provided as a middle step before ordering the final output is important? |
@DeeDeeG Just wow. Yeah looking back through the source, and The order of
A side note that is just unbelievable, seemingly if a property was retrieved via the API that didn't exist within But what that does mean, is the semi-random order of An interesting note. I tried running the new completions.json file locally, and I couldn't replicate the order being retrieved via the specs manually at all. So I thought, maybe But yeah otherwise that's just a mess of behavior. I'm wondering how poorly performance would be if we stop just using |
From the autocomplete-css README.md. |
Wow, when you look everywhere but the readme. Thanks for the link, but that's just gross. Gotta find a solution to that one way or another |
`update.js` now will not overwrite any duplicated property entries. In the case of duplication found will write only the one with the most values. Additionally values are now dedupe prior to adding. Finally we now support Non-Terminal Data Types in CSS Value Definition Syntax, allowing us to resolve values when the value group refers to another property
Co-authored-by: Steven Nguyen <nguyeste008@students.garlandisd.net>
…ompletions from now into the future
Alright, all tests are now finally passing locally on my machine. Essentially the tests for So with that said there's a few things I've done. Unfortunately this makes the tests less readable when they fail, but does mean they will fail significantly less often. In the case of checking the length of completions that would be provided for a character, since of course now there are more completions every single check failed expecting a smaller number than we now have. While there was a whole discussion about the best choice here over on Discord in the end I've opted to do the simplest thing that can hopefully protect against regression. Checked if the amount of completions we have is higher than what was previously expected. This at least ensures the test will pass, that is until they start removing items from the CSS spec faster than they add them. Which I don't personally think is something we need to be super worried about. The other options discussed where ensuring the amount is over 0, or seeing if it's within a certain range. But in the end I do personally feel this is the best solution. But now for the case where the tests would check the exact order of completions. By this I mean there were endless statements like So in that case I've rewritten the majority of tests (whatever was failing) to stop doing this. From what I rewrote we no longer expect any exact order. All we do is check if the value we want to test for exists somewhere in the completions. This is done with I will admit I hadn't wanted to touch specs on this PR, and assumed I wouldn't have to. But from here the last thing to do is remove the note about Side Note: One last thing I had to add was inserting the implicitly defined CSS Values onto all properties, this did cause our |
Note: We also have some draft properties like https://drafts.csswg.org/css-ruby-1/#collapsed-ruby This didn't have a (There sure are a lot of CSS properties out there!) |
Yeah looking at all the empty descriptions, and have gotten nearly all of them solved locally, just taking a look at a last few ones, but should be up within 30 minutes hopefully |
Alright, just pushed some changes, now I'll have to correct myself, when I said "nearly all of them" turns out that was a lie lol. Doing a spot check on those remaining I am able to find them listed in their respective official spec although not on MDN. Really all this whole ordeal is teaching me is maybe I should start contributing to MDN for some of these undocumented elements. If you'd also like to do a spot check on any remaining values the best thing to do is:
|
Some of the I am looking through the Mostly it's the prefixed name minus the Maybe we should programatically generate description strings that just say "alias of: [non-prefixed property name here]"? Except for |
Yeah I've been looking around for a solid solution here. The only thing I can see to find is either we can build a manually maintained list based off the resource you provided, or we accept all of those attributes having no description. Since there are many vendor prefixed items that do have proper descriptions, but many that don't. At this point it really does make it tempting to try and build a much more intelligent parser for this sort of thing, as it's hard to believe no single resource has the information we need in a machine parsable way. But which do you think is better then? Leaving as is or a manually updated list, since we could do either. And if a manual list can always default to assuming it's its own valid vendor prefixed item |
I got this list of remaining properties with empty description Properties with empty
|
I dunno, there's this I guess? https://www.w3.org/TR/CSS/#properties Not super machine-readable, but maybe scrape-able and we can go from there? Maybe the source of this page is checked in to a repo somewhere? Hmm. I see it's checked in here: https://github.com/w3c/csswg-drafts/blob/main/css-2023/Overview.bs#L1045 But there is some magic to it, because it doesn't have anywhere near all the content in that file. I assume a lot is added during the build. I think their CI uses some custom W3C tool called |
@DeeDeeG yeah so looking through the properties left I was able to automatically collect one more. But beyond that it seems none of these items exist on MDN to the best of my ability, so limit our ability to collect them automatically. Despite my best efforts in doing so. So for the time being for some of these last elements I may just go ahead and build a small manual list of descriptions to include. Like we have determined having empty descriptions is not the end of the world, so while in the future this list will not be all inclusive, we can still automatically update these without concern. Adding in the newly empty descriptions is just a cherry on top that wouldn't at all be required. So I'll try to submit a PR with those changes soon, and add what I can. But after all of this I may decide to take the time to try and find a way to get specs like this automatically updated elsewhere so that we here can rely on it, and hopefully anywhere else as well. Since it does seem crazy to me that this isn't easily possible as of right now. |
@DeeDeeG alright, so I've tried my best to curb the issue of many empty property descriptions. Like mentioned above the best way I could see us doing this, is unfortunately a manual list. So I've gone ahead and implemented this behavior, where as a last resort our manual list is checked for a property description. Once the I've now added many manual descriptions either copy/pasted from the source spec or manually written using the source spec as a guide to do so. This has now gotten our empty completion descriptions down from The remaining empty description completionsmargin-break |
@DeeDeeG just an fyi, I've now updated So there should be zero empty descriptions now. |
This reverts commit 2a3afcb.
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.
Okay, thank you for this huge effort. Now with all the descriptions filled out, there's nothing left to object to here, IMO.
I tested the latest binary from this PR, and it's working for me still 👍.
Tried doing node update.js
and got equal lines added/removed, looks like the order just shuffled around (I assume this might be an OS thing? I'm on Linux right now). So, completions appear to be stable (other than some descriptions were slightly updated, maybe from the mdn repo -- EDIT: I am not really concerned about getting these in to merge this PR, it would be okay to, it would be okay not to. There will always be MDN contributions, we can't realistically stay 100% up to date on these.).
I see there are indeed no properties without descriptions. Thank you again for this huge effort.
I have one comment below about a cosmetic/docs type suggestion of which stuff to log when running node update.js
. I don't care strongly about that, it is just something I noticed, feel free to apply it or ignore it/decline to apply it.
Approved for the above reasons, and as mentioned in earlier approvals in the thread.
Co-authored-by: DeeDeeG <DeeDeeG@users.noreply.github.com>
Thanks a ton to everyone for all the help on this one, and to @DeeDeeG for being so persistent with making sure this got reviewed and had an eye kept on it. Appreciate all of you, and with the above approval I'll go ahead and merge! |
Alright so this PR turned into a lot more than I had intended it to be.
In short this PR allows
autocomplete-css
to continue to update it'scompletions.json
as well as getting it updated to today's standards. This was done by completely rewriting theupdate.coffee
and now usingupdate.js
That's the short version, but for some context:
Previously
autocomplete-css
would use this JSON file locatedhttps://raw.githubusercontent.com/adobe/brackets/master/src/extensions/default/CSSCodeHints/CSSProperties.json
to update it's completions. This file would provide it with CSS Properties, which it would then manually query MDN for each property, and scrape the webpage for relevant data to build a description.But as one might imagine this file became out of date, and no longer would provide us with the newest CSS Properties. So rather than try to find someone else that was manually managing this data, I turned to a resource that would keep this data updated for some time, and had at least some reason to keep things backwards compatible when inevitably the data structure changes, hopefully it won't be for some time.
But I ended up using
@webref/css
NPM GitHub which as they define the standards, they will continue to have the most up to date information we could get. This package provides an easy way to get an abundance of data about every possible specification of CSS. More than we needed, but this would give us what we need.The two big hurdles:
The data format here was slightly tricky, requiring a bit of code to work around it, but nothing to crazy. The real difficult part was their usage of CSS Value Definition Syntax, which while obvious that they would use it, meant we couldn't easily grab the data needed for our
completions.json
, that is grab the valid values that each property would accept. Instead of trying to scrape MDN or anything I instead built a super simple parser. This parser is located incssValueDefinitionSyntaxExtractor.js
(A bit obvious and long I know) but this will go ahead and accept any CSS Value Definition Syntax String and parse out all valid values of a property. While this does ignore many aspects of the syntax, such as some values are only valid when another is not present, that was out of the scope of what we needed. And would require significant change on theautocomplete-css
package as a whole, even if in the future I'd love to see this being able to utilize the syntax directly and actually provide autocompletions based on that.The second challenge here was to actually then get descriptions of our properties. This was done utilizing
mdn/content
which is the GitHub repo that contains the Markdown documents of all of MDN's website. With this we are much more easily able to parse out the needed descriptions from the Markdown rather than having to worry about hitting their website dozens of times and dealing with the overhead of that. A quick note, is these documents are also the same way we are collecting our HTML tags for ourcompletions.json
file so those will continue to be updated.The last note here, our
pseudoSelectors
are not updated. At this time I was unable to identify the best way to keep these updated, so for the time being we are still using the same ones that we had previously. There is a function that can be used to start updating them, once we determine the best way. But for now that is not being accomplished.But with all that said, our
pseudoSelectors
have not been changed, but ourproperties
are now as up to date as the current CSS spec is, andtags
are as updated as the MDN website is. So I'd like to think this is a huge improvement over what we previously had, even if I now have ideas for how to improve our package as a whole.Resolves #393