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

fix: use empty string as the default label and check autolabel statements #754

Merged
merged 1 commit into from Jan 5, 2022

Conversation

wodeni
Copy link
Member

@wodeni wodeni commented Jan 5, 2022

Description

Related issue/PR: #752

When NoLabel is used or no labeling statements exist for a Substance variable, the current system doesn't insert a label field for that variable in the Translation. This often causes expected errors. In this PR, we change the default to empty strings as default labels even when there's no labeling statements.

Implementation strategy and design decisions

  • AutoLabel None should define an empty label #752 also uncovers a bug in the Substance checker: we didn't check the variables of statements like AutoLabel A, B, C. Added the checking in the Substance checker.
  • Changed Substance postprocessing to insert default "" labels for every variable and insert "" for NoLabel statements

Examples with steps to reproduce them

@wodeni wodeni self-assigned this Jan 5, 2022
@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #754 (4141aa5) into main (df8d0aa) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
+ Coverage   65.60%   65.61%   +0.01%     
==========================================
  Files          46       46              
  Lines        8144     8147       +3     
  Branches     1525     1525              
==========================================
+ Hits         5343     5346       +3     
  Misses       2792     2792              
  Partials        9        9              
Impacted Files Coverage Δ
packages/core/src/compiler/Style.ts 87.50% <100.00%> (-0.02%) ⬇️
packages/core/src/compiler/Substance.ts 91.17% <100.00%> (+0.13%) ⬆️

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 df8d0aa...4141aa5. Read the comment docs.

@keenancrane
Copy link
Collaborator

Awesome! Looks good. Thank you @wodeni!

@keenancrane keenancrane merged commit 6ce1b97 into main Jan 5, 2022
@wodeni wodeni deleted the fix-no-label branch January 5, 2022 18:30
@keenancrane
Copy link
Collaborator

keenancrane commented Jan 8, 2022

@wodeni Does AutoLabel None no longer work as of this PR? When I add

AutoLabel None

To my Substance program I get

Variable None (at line 12, column 11 of Substance program) does not exist.

Seems it's checking to see if None is a variable name. But shouldn't the only things following AutoLabel be either All or None? (And we only need to check for a variable name after Label, not AutoLabel?)

@wodeni
Copy link
Member Author

wodeni commented Jan 8, 2022

@wodeni Does Autolabel None no longer work as of this PR? I get


Variable None (at line 12, column 11 of Substance program) does not exist.

Autolabel None is never supported iirc.

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.

None yet

2 participants