- I understand why you wanted to rename the function from replace to edit and
your concern makes sense. However, the description of the function still uses
the term replace and I kind of like the explanation as it is.
Sorry about this, this comment was out of date. I put a description that, I
- The comment should mention that you can't replace a node with an object or
vice versa (mentioning inconsistent kind with the ZDDY0040 error is not enough
for the user). Also, that it's only possible to replace root nodes.
I updated this accordingly. Replace root nodes only is handled by
- Could the Item::swap function be implemented on JSONObjects, JSONArrays, and
the base-class for all nodes instead of for Item. It seems like that checking
if a node is the root and swapping the tree in XmlNode::swap would be
sufficient. That seems to be more consistent with the way other
I nevertheless could do some simplification in the code.
--
https://code.launchpad.net/~zorba-coders/zorba/replace-nodes-in-collection/+merge/138196
Your team Zorba Coders is subscribed to branch lp:zorba.
--
Mailing list: https://launchpad.net/~zorba-coders
Post to :
- Is there no more efficient way to find out whether a JSONItem is the root of
a tree or not? Currently, it seems that the searching only works for ordered
collections because it's using findNode.
Yes, this makes sense. I introduced Item::isRoot for this.
--
Voting does not meet specified criteria. Required: Approve 1, Disapprove 1,
Needs Fixing 1, Pending 1. Got: 1 Needs Fixing.
--
https://code.launchpad.net/~zorba-coders/zorba/replace-nodes-in-collection/+merge/138196
Your team Zorba Coders is subscribed to branch lp:zorba.
--
Mailing list:
Review: Approve
--
https://code.launchpad.net/~zorba-coders/zorba/replace-nodes-in-collection/+merge/138196
Your team Zorba Coders is subscribed to branch lp:zorba.
--
Mailing list: https://launchpad.net/~zorba-coders
Post to : zorba-coders@lists.launchpad.net
Unsubscribe :
Review: Approve
Ghislain didn't vote on his own proposal, but I'm assuming he'd approve it. So
I'm adding a second Approve vote in his place. If this is OK, Matthias, go
ahead and merge the proposal.
--
https://code.launchpad.net/~zorba-coders/zorba/replace-nodes-in-collection/+merge/138196
Voting does not meet specified criteria. Required: Approve 1, Disapprove 1,
Needs Fixing 1, Pending 1. Got: 1 Pending.
--
https://code.launchpad.net/~zorba-coders/zorba/replace-nodes-in-collection/+merge/138196
Your team Zorba Coders is subscribed to branch lp:zorba.
--
Mailing list:
Review: Needs Fixing
- The implementation looks very clean. Only a couple of minor comments that
might help removing some code but I couldn't find any major rejection points.
;-)
- ChangeLog entry is missing
- I understand why you wanted to rename the function from replace to edit and
your
Voting does not meet specified criteria. Required: Approve 1, Disapprove 1,
Needs Fixing 1, Pending 1. Got: 1 Pending.
--
https://code.launchpad.net/~zorba-coders/zorba/replace-nodes-in-collection/+merge/138196
Your team Zorba Coders is requested to review the proposed merge of
11 matches
Mail list logo
