v1.6.1 Feature List

[caesar0301]

v1.6.1 Feature List:

• [x] Support subtrees with inter-node pointers (#127)
• [x] Support composite (derived) tree class
• [x] Replace long if-else blocks with look-up tables (#109)
• [x] Shallow pastes to multiple trees breaks level/depth (#88 #131 )
• [x] Ability to paste trees at root level (#87 #134 )
• [x] Fix removal of root node (#119 #130)
• [x] Tree merge feature (#138)
• [x] Optional print in show method (#139)

[caesar0301]

Welcome to join me on branch dev

[leonardbinet]

I'm not sure to get it, looking at dev branch commits I find no trace of my previous commits, did you remove them?

[leonardbinet]

Ah I guess you squashed commits

[caesar0301]

@leonardbinet Yes, I squashed and reviewed the patch last weekend. Your *_in_tree functions are replaced by new predecessor and successors interfaces, which deprecate previous bpointer and fpointer. An optional arg tree_id is appended to functions such as is_leaf and is_root without proposing new interfaces.

TODO: before we finish #127:

1. make tree_id as optional arg in setter and getter of predecessor & successors. #127 is an enhanced feature of logic subtrees. It should not effect the clearness of existing interfaces for some simple use cases.
2. more UTs to cover the potential errors after introducing this tree-wise feature.

[leonardbinet]

@caesar0301 well done for the implementation, I agree with you it is more elegant this way 👍

However why was it necessary for you to squash commits? If you want me to squash my commits in a single one to avoid having too many, no problem I understand 🙂. But the problem here is that you totally deleted my contribution from the history of the repository (as detailed here: https://github.com/isaacs/github/issues/1303).

This quite matters to me, since I see contributions to open-source projects are the main way for me to showcase my capabilities to potential recruitors etc. In this case I won't appear in git blame, history, or contributors of the repo, whereas I spend quite an effort trying to make a meaningful contribution. Would you consider restoring the history?

If so, you could simply repush feature/tree-wise-node-pointer branch (https://github.com/caesar0301/treelib/pull/129) so I can fetch it, squash my commits in a single one, and rebase with your following commits, I would then push this branch in dev2 so you can validate if that suits you.

[leonardbinet]

Actually, I could restore history without requiring the feature/tree-wise-node-pointer branch. I simply squashed my commits and cherry-picked your following ones:

The dev2 branch is now at the exact same point than dev branch, with only difference being my squashed commit 4dde1fd833bb69a5e8375072575c4c091308ad60:

➜  treelib git:(dev2) git fetch upstream
➜  treelib git:(dev2) git reset upstream/dev2
➜  treelib git:(dev2) git status
On branch dev2
nothing to commit, working tree clean
➜  treelib git:(dev2) git reset upstream/dev
➜  treelib git:(dev2) git status
On branch dev2
nothing to commit, working tree clean

I now only need to force-push dev2 branch into dev branch and we're good to go, is it ok for you?

[leonardbinet]

FYI I would rather first solve above situation before merging #130 and #134.

[leonardbinet]

FYI to merge #130 and #134 I pushed the branch with restored history in dev, and kept a copy of your initial branch dev-> dev_save so you can compare.

[leonardbinet]

@caesar0301 do you want to include serialization/deserialization #85 in this release? or should we release the current state and make another release in which we'll have time to discuss this separate subject?

[caesar0301]

@leonardbinet It is fair to remain your efforts on the project through git logs. I compare both dev and dev_save branch and that is OK. I agree with you to start a new release on #85 and #95. Before the release, I should fix the TODOs in https://github.com/caesar0301/treelib/issues/128#issuecomment-564052527 on issue #127.

[leonardbinet]

@caesar0301 for the TODO:

make tree_id as optional arg in setter and getter of predecessor & successors. #127 is an enhanced feature of logic subtrees. It should not effect the clearness of existing interfaces for some simple use cases.

-> I disagree, it is a fix, not an additional feature. If you don't set this tree_id argument you will have unexpected behaviour while using a node in multiple trees (using sub_tree/remove_subtree for instance) methods since a node won't have same predecessor and successors in different trees. And I don't think choosing arbitrary the first tree in which the node has been placed is a good idea (I've commented this subject on this issue: https://github.com/caesar0301/treelib/issues/121).

more UTs to cover the potential errors after introducing this tree-wise feature.

-> IMO it's not that bad but I might help on this, on which methods do you want to increase test coverage?

[leonardbinet]

In the library I plan to release I make use of this feature https://github.com/caesar0301/treelib/pull/138, would you agree to merge such feature in this release?

[caesar0301]

make tree_id as optional arg in setter and getter of predecessor & successors. #127 is an enhanced feature of logic subtrees. It should not effect the clearness of existing interfaces for some simple use cases.

-> I disagree, it is a fix, not an additional feature. If you don't set this tree_id argument you will have unexpected behaviour while using a node in multiple trees (using sub_tree/remove_subtree for instance) methods since a node won't have same predecessor and successors in different trees. And I don't think choosing arbitrary the first tree in which the node has been placed is a good idea (I've commented this subject on this issue: #121).

It is reasonable to think about tree_id when we are talking about multree features. But it doesn't convince me to accept an implementation mannar that expose inner complexities of software or library to users when they merely need basic features in most cases.

I agree with you that it is a fix. It is better to leave it for future release.

[caesar0301]

In the library I plan to release I make use of this feature #138, would you agree to merge such feature in this release?

It seems a helpful feature and not complicated to do. Let's release 161 when you finish this feature.