Skip to content
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

Allow nesting of trees #1059

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

icewind1991
Copy link
Contributor

Makes Tree implement ICollection and modify getNodeForPath to pass the request to nested trees.

My main use case for this in Nextcloud is that for part of the tree we can optimize getNodeForPath by directly getting the target node instead of going trough all parent nodes, but there is currently no clean way to apply this optimization to only a part of the dav tree.
With nested trees we could put this optimization in a separate Tree subclass and nest it into the "main" tree.

@evert
Copy link
Member

evert commented May 24, 2018

I've been considering doing this exact thing for a while, because it kind of allows for a sort of 'mounting' type of system.

However, the methods having multiple purposes (delete() vs delete($path)) makes me feel a bit uncomfortable.

So one thing I'm wondering is, could it make sense to instead just add a new interface that only defines the getNodeForPath() function. (or maybe getChildByPath(), idk), or does that not satisfy your use-case?

@icewind1991
Copy link
Contributor Author

I'm also not a fan of the method name conflicts.

A separate interface for just the getNodeForPath would probably cover the specific use case I started the work for but having full nesting of trees would most likely be useful in future cases (I'm fairly sure it would allow us to do some good code reuse)

@evert
Copy link
Member

evert commented May 24, 2018

Well, adding a separate interface for this one function might simply be a step in the right direction. I don't think we'd paint ourselves into a corner if we do have real use-cases for having the other methods.

@icewind1991
Copy link
Contributor Author

True.

I would go for getNodeForPath instead of getChildByPath or something similar because then Tree can simply implement the new interface.

I'll look into implementing this tomorrow

@codecov-io
Copy link

Codecov Report

Merging #1059 into master will decrease coverage by 0.15%.
The diff coverage is 58.06%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1059      +/-   ##
============================================
- Coverage     97.92%   97.76%   -0.16%     
- Complexity     2867     2880      +13     
============================================
  Files           174      174              
  Lines          7898     7919      +21     
============================================
+ Hits           7734     7742       +8     
- Misses          164      177      +13
Impacted Files Coverage Δ Complexity Δ
lib/DAV/Tree.php 89.68% <58.06%> (-10.32%) 54 <15> (+13)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c077d66...02f6996. Read the comment docs.

@DeepDiver1975
Copy link
Member

@icewind1991 let's revive this once work continues on #1060

@DeepDiver1975
Copy link
Member

@icewind1991 rebase and move on? 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants