-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[YAML] Allow constants and simple comparisons in generic expressions. #31455
Conversation
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
R: @Polber |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
The Java WindowedWordCountIT failures look unrelated. |
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.
Overall, looks really good - my one concern is the clunkiness of string literals... I know that YAML treats everything as a string, so you need some way of emphasizing when an expression should be treated as a string literal, but specifying '"some string"'
feels messy. Is there a way to modify the SafeLineLoader
to preserve explicit double quotes?
Something like https://stackoverflow.com/a/67917266 perhaps
return | ||
|
||
raise ValueError( | ||
f"Missing language specification or unknown input fields: {expr}") |
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.
nit: Should this message refer to expression rather than input fields?
f"Missing language specification or unknown input fields: {expr}") | |
f"Missing language specification or invalid generic expression: {expr}") |
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.
Fixed. I kept the "unkown input fields" as a missing/typo input name is still a quite likely error here we should point to.
# (the# Row(word='License'); you may not use this file except in compliance with | ||
# the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an# Row(word='AS IS' BASIS, |
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.
FYI all of the test files have this error, so not sure what is generating this license, but it appears to not like single quotes
# (the# Row(word='License'); you may not use this file except in compliance with | |
# the License. You may obtain a copy of the License at | |
# | |
# http://www.apache.org/licenses/LICENSE-2.0 | |
# | |
# Unless required by applicable law or agreed to in writing, software | |
# distributed under the License is distributed on an# Row(word='AS IS' BASIS, | |
# (the "License"); you may not use this file except in compliance with | |
# the License. You may obtain a copy of the License at | |
# | |
# http://www.apache.org/licenses/LICENSE-2.0 | |
# | |
# Unless required by applicable law or agreed to in writing, software | |
# distributed under the License is distributed on an "AS IS" BASIS, |
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.
Fixed here and #31480 for the rest.
I agree that it's a bit clunky, but I do want the value to be a string that contains quotes in it as a literal. Shouldn't be too common, and it's what I would also use if I was specifying an explicit language.
This seems to be about preserving the quoting style in writing, but it seems dangerous to treat 'possible_var' and "possible_var" differently in parsing. |
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.
Addressed comments, PTAL.
Co-authored-by: Ahmed Abualsaud <65791736+ahmedabu98@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #31455 +/- ##
============================================
+ Coverage 71.14% 71.15% +0.01%
Complexity 3008 3008
============================================
Files 1055 1055
Lines 133439 133434 -5
Branches 3248 3248
============================================
+ Hits 94929 94948 +19
+ Misses 35382 35358 -24
Partials 3128 3128
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Any more thoughts on this? |
I agree with your comments above - I still think it's clunky, but I agree the use-case is limited and this way it is more explicit. I think biggest thing is to add documentation and possibly adjusting the error message to point out that the expression should include single quotes around a double quoted string for string literals (which could be displayed only if regex does not match it to a number). |
…r-beam into yaml-generic-constants
I added a section to the documentation and referred to that in the error message. |
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.