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

Added depth limit #318

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added depth limit #318

wants to merge 1 commit into from

Conversation

keijokapp
Copy link

This PR adds depth option (and --depth CLI argument). It can be used to remove all modules from the output that are more than depth steps away from the closest input module ("deep dependencies"). depth = 0 makes it to only include input modules.

Note that the whole tree still gets traversed because deep dependencies could reference back to non-deep modules (i.e. modules that are <=depth steps away from any input module). In that case, the intermediate deep dependency is removed and the non-deep dependencies are connected directly. This can cause self-dependent modules.

Example:

depth: 1
input: [A, D]

full graph:
A => B => C => D
output graph:
A => B => D

I haven't written tests yet.

@keijokapp keijokapp force-pushed the depth-limit branch 2 times, most recently from 104e0d1 to 77ddee5 Compare June 24, 2022 17:55
bin/cli.js Show resolved Hide resolved
@PabloLION
Copy link
Collaborator

PabloLION commented Jan 30, 2023

Another thing is about the naming: --depth is slightly more confusing than --max-depth, if consider we add more functionality about depth. For now it's fine, but I still recommend to use --max-depth.

@keijokapp
Copy link
Author

Thanks for the review!

  1. I've rebased the feature branch on top of master
  2. I've changed the if ('depth' in config) checks to if (config.depth) which is more consistent with the other checks and eliminates the confusion with Number('') === 0.
  3. I would advise against using parseInt or parseFloat. Their semantics is even more bizarre than JS's already messed up type conversion, especially that they succeed if only the prefix is parseable and ignore non-numeric suffixes. Number (or unary +) is IMO nearly always a better option.
  4. Regarding changing the name --depth. I don't think max-depth is a good alternative name but it's your decision in the end.
    1. While technically max-depth avoids potential name conflicts in the future by making the name more verbose, it still reseves the word depth to mean the same thing. So using the word depth for other things would be confusing. An alternative would be making the option name [semantically] more specific, like --output-depth (because the option only affects the output and not, for example, traversing depth).
    2. My experience with CLI tools is that --depth are --level are conventional option names for this kind of behaviour. I think that having a prefix there would be slightly awkward. I can come up with one realistic feature that could clash with name depth - traversing depth (for very large code bases where filtering with --exclude wouldn't be easy) - but this would be much more specific use case and would warrant more specifically named option name (--traverse-depth).

@PabloLION
Copy link
Collaborator

Need testcase. I try to add them later.

@miluoshi
Copy link

Any update on this? 👀

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

3 participants