Make ArrayUpdater backwards compatible #4427
Conversation
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained
flow-data/src/main/java/com/vaadin/flow/data/provider/ArrayUpdater.java, line 131 at r1 (raw file):
* {@link JsonCodec} */ void enqueue(String name, Serializable... arguments);
this is also method added from the last commit. cb03998#diff-16bd1774bec0a0a2223b28d5d9b1a294R125
should this be removed ?
gilberto-torrezan
force-pushed
the
gilberto/non_breaking_array_updater
branch
from
da7635e to
48ead47
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained
flow-data/src/main/java/com/vaadin/flow/data/provider/ArrayUpdater.java, line 131 at r1 (raw file):
Previously, ZheSun88 (Sun Zhe) wrote…
this is also method added from the last commit. cb03998#diff-16bd1774bec0a0a2223b28d5d9b1a294R125
should this be removed ?
Good catch. Made it default as the others.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 1 stale
With the HierarchicalArrayUpdater interface, the logic needed for TreeGrid (or any hierarchical listing component) is separated from the ArrayUpater interface, making the ArrayUpater itself backwards compatible.
| @@ -170,9 +168,9 @@ public void reset() { | |||
| public void setParentRequestedRange(int start, int length, T parentItem) { | |||
There was a problem hiding this comment.
|
SonarQube analysis reported 14 issues
Watch the comments in this conversation to review them. Top 10 extra issuesNote: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
There was a problem hiding this comment.
Reviewed 6 of 6 files at r3.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale
a discussion (no related file):
This is kind of step back after the step forward (or the other way around).
Initially there has been almost identical HierarchicalArrayUpdater class (with may be a different name).
Then @tltv merged two classes in one.
Now you split them out back.
I would like to hear @tltv reason of merging those classes.
The backward compatibility of the ArrayUpdater class is quite clear reason for me.
The question is about benefits of another approach .......
I'm not sure we have to keep backward compatibility for the quite internal class.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale
a discussion (no related file):
Previously, denis-anisimov (Denis) wrote…
This is kind of step back after the step forward (or the other way around).
Initially there has been almost identicalHierarchicalArrayUpdaterclass (with may be a different name).
Then @tltv merged two classes in one.
Now you split them out back.I would like to hear @tltv reason of merging those classes.
The backward compatibility of theArrayUpdaterclass is quite clear reason for me.
The question is about benefits of another approach .......
I'm not sure we have to keep backward compatibility for the quite internal class.
The current implementation breaks IronList, making it incompatible with the new Flow 1.1.x, which is not desirable.
The main difference of the previous implementation is actually on the Grid side. I'll make a PR to show what would need to change.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained, and 1 stale
a discussion (no related file):
Previously, gilberto-torrezan (Gilberto Torrezan) wrote…
The current implementation breaks
IronList, making it incompatible with the new Flow 1.1.x, which is not desirable.The main difference of the previous implementation is actually on the Grid side. I'll make a PR to show what would need to change.
I would like to hear @tltv .
But most likely he is on vacation........
So let's go on without waiting then.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained, and 2 stale
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained, and 1 stale
a discussion (no related file):
Previously, denis-anisimov (Denis) wrote…
I would like to hear @tltv .
But most likely he is on vacation........So let's go on without waiting then.
The PR for Grid/TreeGrid is here: vaadin/vaadin-grid-flow#275
There was a problem hiding this comment.
Dismissed @vaadin-bot from a discussion.
Reviewable status:complete! all discussions resolved, 1 of 1 LGTMs obtained, and 1 stale
|
a discussion (no related file): Previously, gilberto-torrezan (Gilberto Torrezan) wrote…
Reason of merging those classes (or adding a TODO comment actually) was purely to get rid of few extra methods and one extra interface by just adding parameters to other. And back then I had no idea that it could broke other modules. |
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, tltv (Tomi Virtanen) wrote…
Reason of merging those classes (or adding a TODO comment actually) was purely to get rid of few extra methods and one extra interface by just adding parameters to other. And back then I had no idea that it could broke other modules.
Well the question was about in preferences one approach against another.
Of course you weren't aware of influence of those changes. So no problem.
Looks like backward compatibility is more important than other reasons. So let's go with that approach which is already merged anyway.
This change is