-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(client): load local file variable #4594
fix(client): load local file variable #4594
Conversation
related to: #4523
Based on the code base we only test basic functionalities for External Client so I have not added / updated any tests. |
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 very nice already!
🔧 I added some minor suggestions.
❓ Do you think it makes sense to do further manual testing by QA?
@@ -84,4 +85,14 @@ public String toString() { | |||
return "DeferredFileValueImpl [mimeType=" + mimeType + ", filename=" + filename + ", type=" + type + ", isTransient=" + isTransient + ", isLoaded=" + isLoaded + "]"; | |||
} | |||
|
|||
@Override |
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.
🔧 Can we move the new getter and setter above the toString()
method? This will be more consistent.
❓ Do you think it makes sense to include processInstanceId
and executionId
in the toString()
result?
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.
Good points, thanks!
- Moved the new getter and setter above the
toString()
method. - I think it can helpful when users use the
toString()
for debugging. Updated thetoString()
to includeprocessInstanceId
andexecutionId
void setExecutionId(String executionId); | ||
|
||
String getExecutionId(); | ||
|
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 should add Javadoc for interface methods.
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.
Added.
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.
Awesome 👍
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.
Two things I think we should reconsider to change: ❌
- Why do we expose setting and getting the execution ID to the user by adding these methods to the public API while we don't do it for the process instance ID?
- Tests are missing.
related to: #4523