-
Notifications
You must be signed in to change notification settings - Fork 323
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
Added depth limit #318
Open
keijokapp
wants to merge
1
commit into
pahen:master
Choose a base branch
from
keijokapp:depth-limit
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Added depth limit #318
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
import 'b.js'; | ||
import 'c.js'; |
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
import 'd.js'; | ||
import 'e.js'; |
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
import 'f.js'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
import 'c.js'; | ||
import 'g.js'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
import 'b.js'; |
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.
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 apologize for my recent lack of activity. I'm currently sorting out a few details, as I've noticed that the implementation of
C, E, F, C
withdepth=3
in the test file seems to induce a self-dependency, which isn't accounted for in the final test case (the highlighted one). I need a bit more time to fully understand this, as I initially thought this PR primarily involved output formatting.Could you @keijokapp please provide a specific use case or example that illustrates the necessity of this functionality? It would greatly help in evaluating its impact and relevance.
Thank you for your understanding and support.
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 might not fully understand the question.
The output of
depth=3
does showc -> e -> f -> c
dependency cycle (?) So do tests withdepth=1
anddepth=2
. In the context of this feature, there isn't anything special about circular dependencies. They are handled just like any other "back-references" (ie references from deep dependencies to non-deep dependencies). If there are circular dependencies in the output, it means there were circular dependency in the original graph. These tests make sure that they are handled (short-circuited) correctly.You mean "circular dependencies"? I don't think I'm the right person to explain the use cases for circular dependencies because I personally have a strong distaste for them. But they are nevertheless common and in rare cases not easily avoidable. This tool specifically handles them eg. by coloring them red on the dot graph. The feature doesn't have anything specifically to do with circular dependencies.
When it comes to "limiting the depth", there are two ways to go about it. One is to stop traversing at
depth
and ignore all references originating from that point onward. This is not usually what the user wants. Not showing those "back-references" gives a wrong impression about the dependency structure. For example, atdepth=1
, the user might seeb.js
andc.js
as fully independent branches while, in fact,b.js
is an indirect dependency ofc.js
.Instead, as noted in the original submission, this feature traverses through the whole tree and does not hide those indirect relationships. It only removes the intermediate deep nodes.
It really is just about the output. It is to remove the "noise" while keeping the useful relationship information. It allows the user to focus around the close proximity of specific modules. It doesn't change anything else about the tool (eg. module resolution, parsing, traversing).
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.
Thank you for your detailed response, and I appreciate your patience as I navigate through this PR. After multiple reviews, I'm still grappling with some aspects of the implementation and would like to discuss these further.
File Structure and Depth Concerns
My apologies for any confusion caused by the
C-D-E-C
example I initially suggested—it may not have been the best illustration of the concept. Here’s a refresher on the file structure from our tests:This tree structure helps illustrate potential dependency chains, such as
A-C-E-F
is like theA => B => C => D
mentioned in the PR description. When discussingdepth=1
, I find that the outputs might not align with what one would expect, likeA-C-D
in the example in the description, which would beA-E-F
in our test case accordingly. This which should help us better understand the effects of limiting depth. Could we consider revisiting this part to ensure it aligns more clearly with our objectives?Circular Dependencies
I generally strive to avoid circular dependencies, too. I found this repository trying to include checks for these circular dependencies in the CI/CD process. Kudos to the contributor for implementing such a valuable feature!
Clarification on the 'depth' Functionality
Could you possibly provide more concrete examples or use cases for the
depth
functionality? While your explanation was insightful, additional examples would help solidify my understanding of its practical impact.Depth Traversal Options
Concerning your point on depth traversal:
I slightly disagree here. In some scenarios, particularly during refactoring, it could be beneficial to understand the breadth of a module's direct influence, which would support limiting traversal as a feature.
Confusion Over Terminology
These two terms in the description seem to mean the same thing, leading to some confusion. Could you clarify these definitions?
I've noticed significant changes in the
convertTree(depTree, tree, pathCache)
method, particularly the removal ofpathCache
. This alteration could potentially lead to increase execution times significantly. I believe reintroducingpathCache
might enhance performance. I'll need some more time to evaluate this change thoroughly.Thank you once again for your support and understanding as we refine this 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.
I added a few nodes for the tree so that we can be on the same page with "deep dependencies"
For my understanding, with
depth=1
, we are excluding fileb
,c
. And withdepth=2
we are also excludingh
,d
ande
in addition. But hereh
would appear again in the chainACEFGH
which made the output unclear.(This also shows that removing the
pathCache
might be a necessity to implement this feature, depending on what we are trying to achieve.)