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

#367 Use scope of environment for ParseValue in Until instead of the direct name. #368

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

mvanaken
Copy link
Contributor

@mvanaken mvanaken commented Mar 1, 2023

Due to this bug you are unable to select a specific value from the ParseGraph in an other structure.

Included a test, which failed without the fix.

Resolves #367

Copy link
Contributor

@jvdb jvdb left a comment

Choose a reason for hiding this comment

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

Kind of mindblowing that the original implementation of Until just completely threw away the scope's name an inserted its own short name instead.

The tests clearly illustrate that the current workings of Until are not really in line with the general ideas we normally espouse around how combinators and values should interact in Metal. You can see this in UntilTest: the name "struct.value.terminator" doesn't make sense (the terminator is not part of the value!) but it's a result of the fact that Until acts as both a combinator (putting its name in the enclosing scope) and a value definition (putting its name on the parsed value).

Copy link
Contributor

@rdvdijk rdvdijk left a comment

Choose a reason for hiding this comment

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

Great find!

@@ -127,4 +128,16 @@ private Token createToken(final ValueExpression initialSize, final Token termina
return repn(until("line", initialSize, terminator), con(3));
}

@Test
public void nameScope() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Woah 😲

@mvanaken
Copy link
Contributor Author

mvanaken commented Mar 2, 2023

Ran code coverage Jacoco locally, and everything was 100%. This codecov hickup will be resolved in #362.

@mvanaken mvanaken merged commit e3a38c6 into master Mar 2, 2023
@mvanaken mvanaken deleted the until-name-scope-bug branch March 2, 2023 14:06
@mvanaken mvanaken modified the milestone: 10.0.0 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Bug in Until: the name does not contain the scope.
3 participants