-
Notifications
You must be signed in to change notification settings - Fork 78
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 logic to create @webref/css package #115
Conversation
Code follows the same structure as for the `@webref/idl` package. Tests make sure that all CSS fragments can be parsed with the Value Definition Syntax parser of CSSTree. Note that this is not yet the case, there are 10 cases with syntax errors that require the creation of dedicated patches, not included in this commit.
The patches cover obvious grammar issues.
The patches cover constructs that the parser does not yet support and/or that the Value Definition Syntax should be clearer about.
@foolip, I believe that's now ready to merge. I completed the pull request with CSS patches. Some of them are needed because the parser does not support certain constructs that could nevertheless be legit, see discussion in w3c/reffy#494 (comment) I would address the possible addition of CSSOM generated IDL, discussed in #111 (comment), separately. |
@tidoust I'm out this week so please feel free to merge and release the first version without waiting for me! |
ed/csspatches/fill-stroke.json.patch
Outdated
Subject: [PATCH] Separate stacked combinators | ||
|
||
The parser does not support stacked combinators, see discussion in: | ||
https://github.com/w3c/reffy/issues/494#issuecomment-790713119 |
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.
I'm a bit reluctant to integrate patches that target the parser limitations rather than spec bugs; or more specifically, I don't like the idea of packaging/releasing wrongly-fixed spec data
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.
I'm reluctant too but what alternative do you suggest while this gets sorted out?
Drop the value
fields? Drop the properties? Drop the files?
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.
patch the parser :)
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.
I don't think I'll have the bandwidth to look into that in the next few days, but please do :)
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.
Could we drop this patch and initially not guarantee that everything is possible to parse?
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.
having started to look into patching the parser, I agree that this sounds like the best path forward to get something already useful out sooner rather than later
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.
I dropped the patches. Tests ignore the problematic entries for the time being.
- Bikeshed fixed for css-backgrounds - PR merged for css-tables
The README links to the Web IDL patches documentation for details on how to create and maintain the patches.
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.
Looking forward to seeing this published :)
Co-authored-by: Philip Jägenstedt <[email protected]>
Co-authored-by: Philip Jägenstedt <[email protected]>
Co-authored-by: Philip Jägenstedt <[email protected]>
Co-authored-by: Philip Jägenstedt <[email protected]>
Co-authored-by: Philip Jägenstedt <[email protected]>
Co-authored-by: Philip Jägenstedt <[email protected]>
Co-authored-by: Philip Jägenstedt <[email protected]>
Trying to write some code to solve foolip/mdn-bcd-collector#705, I wrote what could become consistency tests: The output of that and the many manually added valuespaces could form a burndown list of what needs to be fixed in specs and possibly in Reffy to extract something that's consistent. A good example is that https://drafts.csswg.org/css-shapes-1/#typedef-basic-shape doesn't define the syntax as "<inset()> | <circle()> | <ellipse()> | <polygon()> | <path()>" but merely has a list of allowed values. |
Co-authored-by: Philip Jägenstedt <[email protected]>
The problematic entries are explicitly listed in the tests for now. Hopefully, we'll be able to drop this (and the caveat in the README) soon.
I was using a locally patched version of CSSTree so did not notice that one...
We did not maintain that list, which is now likely outdated, but we had done a similar exercise in Reffy with a list of missing value definitions. |
I believe I integrated the feedback received. @foolip, how does that look now? |
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.
LGTM % nits
@@ -0,0 +1,43 @@ | |||
# CSS definitions of the web platform | |||
|
|||
This package contains CSS property definitions scraped from the latest versions of web platform specifications in [webref](https://github.com/w3c/webref), with fixes applied to ensure ([almost](#guarantees)) all CSS value definitions can be parsed with [CSSTree](https://github.com/csstree/csstree). |
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.
Do we still have this guarantee? If not this and the guarantees section below needs updating.
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.
That is why I added the "(almost)" with a link to the guarantees section, which explains that there remains a handful of exceptions in practice.
Co-authored-by: Philip Jägenstedt <[email protected]>
Co-authored-by: Philip Jägenstedt <[email protected]>
Co-authored-by: Philip Jägenstedt <[email protected]>
Code follows the same structure as for the `@webref/idl` package. Tests make sure that all CSS fragments can be parsed with the Value Definition Syntax parser of CSSTree. A couple of CSS patches need to be applied to the extracts to fix value definitions. Tests currently only guarantee that the value definitions can be parsed with CSSTree, with the exception of a handful value definitions that are a priori valid but that cannot be parsed with the CSSTree parser for the time being, see discussion in: w3c/reffy#494 (comment) Co-authored-by: Philip Jägenstedt <[email protected]>
Code follows the same structure as for the
@webref/idl
package. Tests make sure that all CSS fragments can be parsed with the Value Definition Syntax parser of CSSTree.Note that this is not yet the case, there are 10 cases with syntax errors that require the creation of dedicated patches, not included in this pull request.
Actually, I realize that I don't know how to generate patch files. @foolip, what's the best way to do that?
I kept the logic in the
index.js
file simple and in particular it does not have any logic to actually parse the CSS fragments. I just put an example in the README. Let me know if you believe theparseAll
function should also createparsedValue
properties.Related discussion in #111, also in Reffy in w3c/reffy#494