-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fixes structs to handle non-text struct fields. #450
Conversation
Codecov Report
@@ Coverage Diff @@
## main #450 +/- ##
============================================
- Coverage 82.35% 82.34% -0.02%
- Complexity 1394 1399 +5
============================================
Files 171 171
Lines 10717 10742 +25
Branches 1766 1770 +4
============================================
+ Hits 8826 8845 +19
- Misses 1350 1355 +5
- Partials 541 542 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 have some minor suggestions to adjust the error message and error location. Also may need a couple more tests with different struct field name types.
Property.LINE_NUMBER to 1L, | ||
Property.COLUMN_NUMBER to 8L |
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.
There's some slight inconsistency with the error location. If the non-text struct field name is caught at compile time (as in the previous two tests), the error points to the field name. Whereas, in this case it points to the start of the struct constructor (L8). I think both cases should be consistent with the error pointing to the problematic struct field name.
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.
metas
are not available with the ExprValue
once it is evaluated.
Property.COLUMN_NUMBER to 2L | ||
) | ||
) | ||
|
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.
May want to also add some tests with NULL
and MISSING
as the struct field name.
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.
Looks good. Only have a couple small nits and an unused file.
For future tracking: when the evaluator is to support a permissive typing mode, these fields with a non-string struct key would be ignored. From the spec
|
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.