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

feat: check existence and type of labels in Style selectors #777

Merged
merged 6 commits into from Jan 7, 2022

Conversation

wodeni
Copy link
Member

@wodeni wodeni commented Jan 7, 2022

Description

Closes #774

In this PR, we introduce a new Style selector pattern: X has <math|text> label. When included in the where clause of a selector, the compiler checks if label exists and whether it matches with the specified descriptor. Examples:

  • Label x $\vec{x}$ will match with where x has math label
  • Label x "xxxx" will match with where x has text label
  • Both of the above will match with where x has label
  • NoLabel x will not match with where x has label nor where x has math/text label

Error handling:

  • Only math and text are valid field descriptors. All others will result in parse errors
  • label is the only field name allowed, but instead of throwing a parse error, the compiler reports SelectorFieldNotSupported error:
Cannot match on field x.blah (line 14, column 31 of Style program) because matching on fields is not fully supported. Currently, only "label" can be matched in selectors.

Implementation strategy and design decisions

  • Introduce rel_field rule in the Style parser
  • Add RelField type in the Style compiler
  • In Substance postprocessing, attach the type of the label to entries in labelMap
  • In the Style compiler, check for the existence of x and the label existence and type during selector matching
  • Add tests in Style parser, Substance compiler, and Style compiler

Examples with steps to reproduce them

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing tests pass locally using yarn test
  • I ran yarn docs and there were no errors when generating the HTML site
  • My code follows the style guidelines of this project (e.g.: no ESLint warnings)

Open questions

There's no infrastructure for testing selector matching results, so we are not including tests for checking selector matching results.

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jan 7, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: a280fce
Status: ✅  Deploy successful!
Preview URL: https://ce67bdce.penrose-panes.pages.dev

View logs

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #777 (a280fce) into main (52c8005) will decrease coverage by 0.15%.
The diff coverage is 56.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #777      +/-   ##
==========================================
- Coverage   65.56%   65.40%   -0.16%     
==========================================
  Files          53       53              
  Lines        7939     7978      +39     
  Branches     1460     1409      -51     
==========================================
+ Hits         5205     5218      +13     
- Misses       2725     2751      +26     
  Partials        9        9              
Impacted Files Coverage Δ
packages/core/src/compiler/Style.ts 86.24% <53.12%> (-1.52%) ⬇️
packages/core/src/utils/Error.ts 34.64% <60.00%> (+0.56%) ⬆️
packages/core/src/compiler/Substance.ts 91.20% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52c8005...a280fce. Read the comment docs.

@samestep
Copy link
Collaborator

samestep commented Jan 7, 2022

Ah so you changed if/else if/else to switch to avoid having the else case which throws? Nice 👌

@wodeni
Copy link
Member Author

wodeni commented Jan 7, 2022

Ah so you changed if/else if/else to switch to avoid having the else case which throws? Nice 👌

Yep! Still puzzling why there's a distinction in the first place, but yeah it's better to have static errors when types change, instead of these weird exceptions.

I'm just change them as I go. There are a lot more in the repo. Maybe do a full pass later.

Copy link
Collaborator

@samestep samestep left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

@wodeni wodeni merged commit 9357e4e into main Jan 7, 2022
@wodeni wodeni deleted the check-labels branch January 7, 2022 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to match on existence of a label in Style
2 participants