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
feat: Add signal commands #20876
base: main
Are you sure you want to change the base?
feat: Add signal commands #20876
Changes from all commits
95949da
eea8740
371348d
da62c09
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 be marked with
@Nullable
fromJSpecify
(it should be in Flow codebase already, it was added with the CRUD repos feature)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.
Not sure if I easily understand this part:
The description of the
after
param says: id of the node to insert immediately after...and description of the
before
param says: id of the node to insert immediately before...What is the role of the
ZERO
? Is theZERO
an always existing root pointer that will store the head position (or the head == tail == ZERO in case of an empty list)?If so, calling
first()
returns the before first (after the ZERO) position which makes sense. But, thelast()
returns before the ZERO, which I don't simply get :) Was it supposed to beId.MAX
?Probably, something in the implementation is being optimized by this(?), but this representation seems a bit confusing.
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.
This is a bit of (premature ?) optimization: ZERO is widely used also in other contexts and has therefore a custom JSON representation to be as compact as practically possible (
""
).ZERO represents both edges of the list, i.e. both head and tail. This should be safe since they can never be mixed up with each other. Furthermore, ZERO also represents the root node but that node can never have siblings.
I agree that it can be confusing. I'm just not sure if that should be addressed by additional documentation or by removing the optimization? Or maybe just introduce an
EDGE
constant that refers to the sameId
instance? It would probably be dangerous to introduceHEAD
andTAIL
constants with identical values since someone might assume them to be different?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.
Well, at this point, not seeing the rest of the implementation / usages makes it hard to make a practical decision about which approach to pick. Maybe I was mentioned and I forgot: is the List implementation going to be a circular linked list? In the case, it makes sense to have only one EDGE and the before / after seems to be enough.
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.
What would
first()
andlast()
mean in a circular linked list?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, when I was writing the previous comment, I got confused for a sec with having one EDGE constant for both head and tail (for the before first and after last positions), vs. head and tail pointing to the same location all the time (which wasn't the case). I would say, having a separate constant such as EDGE is enough for not getting confused with the ZERO thingy. Having separate
HEAD
andTAIL
constants with different values might not add much value.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.
Looking at the implementation of
TreeRevision # assertValidTree
method, and the way it had to concat thelistChildren
andmapChildren
, seems like a flag to me. Enabling the caller code to provide both, can easily result in creating an invalid Data node. One should be calculated based on the other, for instance, If themapChildren
changes to an orderedMap such asLinkedHashMap
, then thelistChildren
could be just a public method that calculates based onmapChildren.values()
. Or maybe I misunderstood the purpose of having both themapChildren
andlistChildren
side by side.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.
Some child nodes are accessed by key and some by order. There's no reason why a single node couldn't have children of both types even though that's not the typical case.
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.
What I'm not understanding is: should or shouldn't the
listChildren
andmapChildren.values()
contain the same set of Ids?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.
There should be no overlap. Every child should be in exactly one location - either addressable by
ListPosition
or by aString
key but never both at the same time.All operations that move a child remove the child from its current location before attaching it back again, even if moving within the same parent. Any accidental overlap should trigger an error in
TreeRevision.assertValidTree()
from the!visited.add(id)
check (since the concatenation doesn't dodistinct()
). Should maybe add a unit test for that case as well.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.
To me, the it definitely worth adding this to the javadocs.
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.
Same here. Though, I prefer to get rid of this almost duplicate block.
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.
What does
a sequenced map
mean here? Is it referring to the Java 21'sSequencedMap
or an ordered map? If so, shouldn't we define with a more specific interface e.g.SequencedMap
or the good oldLinkedHashMap
?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.
Cannot use
SequencedMap
since we still support Java 17. Don't want to useLinkedHashMap
in any signature since that's an implementation detail.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.
Then what prevents the caller code from passing an unordered map?
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.
If caller code doesn't care about ordering, then it can do whatever it wants. But we should probably make sure there are tests to verify that order remains preserved once it has been implicitly or explicitly established.
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.
Are those tests going to be added to this PR?
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.
Yes. I will add those tests along with some other test omissions that have been pointed out in other comments.