Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[NU-1800] Add template lazy param #7162
base: staging
Are you sure you want to change the base?
[NU-1800] Add template lazy param #7162
Changes from all commits
d312b1a
01f949b
b62efde
a7c4ee6
7d06c80
8d630c1
8bc5ecd
9e917bc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 63 in engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/TemplateLazyParameterTest.scala
GitHub Actions / REPORT-BackendTests-2.13
pl.touk.nussknacker.engine.flink.TemplateLazyParameterTest ► should use spel template ast operation parameter
Raw output
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.
Remove debug println statements from production code
Production code should not contain debug print statements. Consider using proper logging if debugging information is needed.
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.
Implement proper sink functionality
The current sink implementation only prints to console and effectively discards the data. Consider:
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.
We don't want to evaluate it during each invocation
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.
Add error handling for expression evaluation
The expression evaluation should handle potential errors to prevent runtime exceptions.
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.
We can get rid of this exceptions by moving this logic to factory method and check
templateSubexpressions
instead of expression langageThere 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.
🛠️ Refactor suggestion
Add error handling for type casting
The unsafe cast to type T could fail at runtime. Consider adding proper error handling.
📝 Committable suggestion
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.
Review pattern matching order for potential issues.
The
case p: CustomLazyParameter[T]
might catch instances ofEvaluableLazyParameterCreator
before they reach the more specific case above, potentially preventing proper evaluation of nested creators. Consider:EvaluableLazyParameterCreator
case, orEvaluableLazyParameterCreator
Here's a suggested fix:
📝 Committable suggestion
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 think that it should be rather called either parseExpression or parseParts
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.
It's not a template expression