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

[Tree] Add pre-order and level-order traversals #2498

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Jean-Beru
Copy link
Contributor

In order to get the next node when traversing a tree, this PRs add new methods NestedTreeRepository::getNextNode and NestedTreeRepository::getNextNodes. They take some arguments :

  • $root is the top node and is used to limit result to this (sub-)tree
  • $node is the current node (or null for the first call)
  • $limit (only for getNextNodes) can limit nodes count to return
  • $traversalStrategy accepts self::TRAVERSAL_PRE_ORDER (default) or self::TRAVERSAL_LEVEL_ORDER and is used to determine which algorithm use (see https://www.geeksforgeeks.org/tree-traversals-inorder-preorder-and-postorder).

These methods can be used, for example, to read a book page after page:

  • The History of Middle-earth
    • The Book of Lost Tales 1
      • Chapter 1
        • Page 1
        • Page 2
        • Page 3
        • ...
      • Chapter 2
        • Page 1
        • Page 2
        • Page 3
        • ...
    • The Book of Lost Tales 2
    • ...

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2022

Codecov Report

Attention: Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.30%. Comparing base (f21d888) to head (0037f83).
Report is 135 commits behind head on main.

Files Patch % Lines
...rc/Tree/Entity/Repository/NestedTreeRepository.php 95.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2498      +/-   ##
==========================================
+ Coverage   79.22%   79.30%   +0.07%     
==========================================
  Files         161      161              
  Lines        8415     8455      +40     
==========================================
+ Hits         6667     6705      +38     
- Misses       1748     1750       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@morban
Copy link

morban commented Apr 25, 2023

It will be nice if this PR was successful and merge. Thank you.

src/Tree/Entity/Repository/NestedTreeRepository.php Outdated Show resolved Hide resolved
src/Tree/Entity/Repository/NestedTreeRepository.php Outdated Show resolved Hide resolved
src/Tree/Entity/Repository/NestedTreeRepository.php Outdated Show resolved Hide resolved
src/Tree/Entity/Repository/NestedTreeRepository.php Outdated Show resolved Hide resolved
src/Tree/Entity/Repository/NestedTreeRepository.php Outdated Show resolved Hide resolved
src/Tree/Entity/Repository/NestedTreeRepository.php Outdated Show resolved Hide resolved
src/Tree/Entity/Repository/NestedTreeRepository.php Outdated Show resolved Hide resolved
src/Tree/Entity/Repository/NestedTreeRepository.php Outdated Show resolved Hide resolved
src/Tree/Entity/Repository/NestedTreeRepository.php Outdated Show resolved Hide resolved
tests/Gedmo/Tree/NestedTreeTraversalTest.php Outdated Show resolved Hide resolved
@phansys phansys requested a review from franmomu August 1, 2023 09:34
Copy link
Collaborator

@franmomu franmomu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can add object type to parameters

@@ -58,6 +58,7 @@ a release.

### Added
- Tree: Add `Nested::ALLOWED_NODE_POSITIONS` constant in order to expose the available node positions
- Tree: Add pre-order and level-order traversals (#2498). Add `getNextNode()` and `getNextNodes()` methods to traverse the tree
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved to the ## [Unreleased] section

*
* @throws InvalidArgumentException if input is invalid
*/
public function getNextNodesQueryBuilder($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): QueryBuilder
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function getNextNodesQueryBuilder($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): QueryBuilder
public function getNextNodesQueryBuilder(object $root, ?object $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): QueryBuilder

Should we make this method private?

* @param int|null $limit Maximum nodes to return. If null, all nodes will be returned
* @param self::TRAVERSAL_* $traversalStrategy Strategy to use to traverse tree
*/
public function getNextNodesQuery($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): Query
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function getNextNodesQuery($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): Query
public function getNextNodesQuery(object $root, ?object $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): Query

* @param object|null $node Current node. If null, first node will be returned
* @param self::TRAVERSAL_* $traversalStrategy Strategy to use to traverse tree
*/
public function getNextNode($root, $node = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): ?object
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function getNextNode($root, $node = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): ?object
public function getNextNode(object $root, ?object $node = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): ?object

*
* @return array<object>
*/
public function getNextNodes($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): array
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function getNextNodes($root, $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): array
public function getNextNodes(object $root, ?object $node = null, ?int $limit = null, string $traversalStrategy = self::TRAVERSAL_PRE_ORDER): array

@Jean-Beru
Copy link
Contributor Author

@franmomu I agree to add the object type to arguments and make some functions private. Since this logic could be applied to other NestedTreeRepository functions (get*QueryBuilder and get*Query), should we:

  • keep as it is to have the same approach
  • make only new typehinted and private
  • make all typehinted and private => will cause a BC break
    ?

IMO, the second one it the best answer since we improve code without any BC break. Before doing any commit, is it OK for you @phansys ?

}
} elseif (self::TRAVERSAL_LEVEL_ORDER === $traversalStrategy) {
if (!isset($config['level'])) {
throw new \InvalidArgumentException('TreeLevel must be set to use level order traversal.');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why \InvalidArgumentException? $config['level'] isn't provided as an argument in this method.

@phansys
Copy link
Collaborator

phansys commented Aug 12, 2023

IMO, the second one it the best answer since we improve code without any BC break. Before doing any commit, is it OK for you @phansys ?

LGTM.

@phansys phansys self-requested a review August 18, 2023 00:58
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added Stale and removed Stale labels Feb 14, 2024
@Jean-Beru
Copy link
Contributor Author

github-actions, you can keep this PR opened. I'll fix conflicts soon.

@franmomu franmomu added the Still Relevant Mark PRs that might've been auto-closed that are still relevant. label Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Rebase New Feature Still Relevant Mark PRs that might've been auto-closed that are still relevant. Tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants