Refactor MutableTreeRevision #21000
Refactor MutableTreeRevision #21000
Conversation
Legioth
commented
Feb 14, 2025
Legioth
commented
- Rename TreeManipulator to ResultBuilder and use it only for multi-step commands and commands that affect multiple nodes
- Explicitly return a result when handling a command instead of indirectly producing one
- Transactions produce multiple results so they have custom top-level handling instead of following the general pattern
- Handle errors caused by concurrent modifications by returning a Reject result instead of passing the error through an instance field
* Rename TreeManipulator to ResultBuilder and use it only for multi-step commands and commands that affect multiple nodes * Explicitly return a result when handling a command instead of indirectly producing one * Transactions produce multiple results so they have custom top-level handling instead of following the general pattern * Handle errors caused by concurrent modifications by returning a Reject result instead of passing the error through an instance field
| if (mapUpdater.apply(map) instanceof Reject error) { | ||
| return error; | ||
| } |
There was a problem hiding this comment.
Compiler complains about Java 17 compatibility of this expression, considering it to be an unconditional pattern matching instanceof which is supported in Java 21+:
/vaadin/flow/signals/src/main/java/com/vaadin/signals/impl/MutableTreeRevision.java:211:39
java: unconditional patterns in instanceof are not supported in -source 17
(use -source 21 or higher to enable unconditional patterns in instanceof)
Same applies everywhere in this file where (... instanceof Reject error) is used.
Since the updater function is returning CommandResult.Reject, a null check is enough without instanceof.
There was a problem hiding this comment.
Silly of Java to not realize that the expression isn't actually unconditional. Uglified to be compatible with ancient compilers.
There was a problem hiding this comment.
Is this refactoring going to be back ported to V24 branches, or could the original code with the cleaner instanceof syntax be back?
|
|
|
|
Are you working on this or should we close it? |
Test Results1 310 files 1 310 suites 1h 14m 12s ⏱️ Results for commit 50ca9c6. ♻️ This comment has been updated with latest results. |
|
I'm just waiting for someone to review it... |
|
