Update ac-css-property-alist with css3 props (from company) #520
Conversation
piotrklibert
changed the title
|
Also, @jcs090218 would you mind if I moved the CSS source to a separate file? Or more generally - would splitting AC over a number of smaller files be a problem? If not - I'd like to do so. "Better code organization" is often just a synonym for bikeshedding, but here my goal is to enable lexical scoping for the package. I think this is better done step-by-step instead of all at once (ie. adding |
|
I would love to see "Better code organization". Feel free to split the module into pieces if you think it's needed. |
* replaced string regexes with `rx' representations * added cache for possible values * returned candidates are unique but unsorted * added space char to prop-name-only-re so that completion doesn't start immediately after semicolon (;)
|
Ok, so I ended up rewriting most of the code for this source. It's functionally equivalent, but it should be easier to read. Things that happened in this PR:
@jcs090218 please take a look. The logic part of the code is just under 100 loc, and it's moved to the top of the file for easier reading. That being said: this is all pointless. The code is written with CSS2 in mind, and adding a few of the newer properties is not enough to reliably complete in buffers with modern CSS. CSS3 is vastly more complex than CSS2, with media queries, variables, functions, and so on. We're not worse than company right now, but to really make CSS completions useful we'd need to write a parser for CSS first - a few regexes alone won't cut it. As such, I think it would be better to remove this source from AC and move it to a separate package sometime in the future - if we can find someone who'd like to maintain it, that is. |
ac-source-css.el
Outdated
| @@ -0,0 +1,387 @@ | |||
| ;; -*- mode: emacs-lisp; lexical-binding: t -*- | |||
There was a problem hiding this comment.
You might want to add the header as well. Make sure you follow the multiple files package convention. See here.
There was a problem hiding this comment.
Of course. Thanks, I totally forgot about it. I'll have some time to work on this next weekend probably - will fix this and rename the file as you suggested then.
|
Thanks for the changes. First thing came up from my mind is the CI check. Just opened up a new issue for it #521. |
|
I wholeheartedly agree - the CI is needed, and we need a lot more tests than we currently have. I'll see what I can do on that front after this PR :) |
I have added the basic CI from the previous test. I agreed that we do need more tests, but we can add those tests later on. 👍 |
|
Sorry for the delay, I was sick last week and couldn't do much. I renamed the file and added licensing info, but I also started working on stealing the code from company-css for completing CSS pseudo-classes and HTML tags where it makes sense, ie. outside the bodies of the rules. I should get it done in a few days at most :) |
In the previous PR (#476) the author deleted the source repository and left us with raw patches only. I made a branch and applied the changeset from that PR. I'm not sure if it should be merged as-is. One thing that seems strange is the inclusion of parens (and commas) in some candidates, like
matrix(,,,,,). From the diff it looks like we didn't have candidates like this before. Also, I imagine completing such a function would put the point after the closing paren - you'd then have to jump back to the beginning of the arguments list, which could be more distracting than completing tomatrix|and pressing(from there. Things likematrix(,,,,,)are probably better suited foryasnippetthan AC. In any case - we now have a PR that is merge-able, whether we do merge it or not :)