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

Add logic to create @webref/css package #115

Merged
merged 29 commits into from
Mar 10, 2021
Merged

Add logic to create @webref/css package #115

merged 29 commits into from
Mar 10, 2021

Conversation

tidoust
Copy link
Member

@tidoust tidoust commented Mar 3, 2021

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 the parseAll function should also create parsedValue properties.

Related discussion in #111, also in Reffy in w3c/reffy#494

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.
@tidoust tidoust requested a review from foolip March 3, 2021 16:47
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.
@tidoust
Copy link
Member Author

tidoust commented Mar 4, 2021

@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.

@foolip
Copy link
Member

foolip commented Mar 4, 2021

@tidoust I'm out this week so please feel free to merge and release the first version without waiting for me!

Subject: [PATCH] Separate stacked combinators

The parser does not support stacked combinators, see discussion in:
https://github.com/w3c/reffy/issues/494#issuecomment-790713119
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

patch the parser :)

Copy link
Member Author

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 :)

Copy link
Member

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?

Copy link
Member

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

Copy link
Member Author

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.

tidoust and others added 4 commits March 4, 2021 18:31
- 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.
Copy link
Member

@foolip foolip left a 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 :)

packages/css/index.js Outdated Show resolved Hide resolved
packages/css/README.md Outdated Show resolved Hide resolved
packages/css/README.md Outdated Show resolved Hide resolved
test/css/validate.js Outdated Show resolved Hide resolved
test/css/validate.js Outdated Show resolved Hide resolved
test/css/all.js Show resolved Hide resolved
packages/prepare.js Outdated Show resolved Hide resolved
packages/prepare.js Outdated Show resolved Hide resolved
packages/prepare.js Outdated Show resolved Hide resolved
packages/css/README.md Outdated Show resolved Hide resolved
tidoust and others added 9 commits March 10, 2021 10:10
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
@foolip
Copy link
Member

foolip commented Mar 10, 2021

Trying to write some code to solve foolip/mdn-bcd-collector#705, I wrote what could become consistency tests:
9ea71c1

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.

packages/css/README.md Outdated Show resolved Hide resolved
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...
@tidoust
Copy link
Member Author

tidoust commented Mar 10, 2021

Trying to write some code to solve foolip/mdn-bcd-collector#705, I wrote what could become consistency tests:
9ea71c1

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.

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.

@tidoust
Copy link
Member Author

tidoust commented Mar 10, 2021

I believe I integrated the feedback received. @foolip, how does that look now?

Copy link
Member

@foolip foolip left a 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).
Copy link
Member

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.

Copy link
Member Author

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.

packages/css/README.md Outdated Show resolved Hide resolved
packages/css/README.md Outdated Show resolved Hide resolved
packages/css/README.md Outdated Show resolved Hide resolved
test/css/all.js Show resolved Hide resolved
tidoust and others added 3 commits March 10, 2021 14:14
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
@tidoust tidoust merged commit 94aa0ae into master Mar 10, 2021
@foolip foolip deleted the package-css branch March 10, 2021 14:20
tidoust added a commit that referenced this pull request Apr 15, 2021
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 <philip@foolip.org>
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.

3 participants