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.
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
refactor(unstable): deno info --json output #7417
refactor(unstable): deno info --json output #7417
Changes from 4 commits
8e42efe
bfe8be8
051f204
682b7ae
febe905
14bf3fc
e037529
3d78c1a
7b0fe3b
5ae2413
2ef9adf
e5076c0
3e12f8b
97e5938
6852111
a5c3032
ea4870f
83271fe
636de26
11767b2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We don't show
total
size of vertex in terminal output and I think we should skip it in JSON as well. It's useful to have total size of all dependencies in top level structure, but calculating sizes of subdependencies should be left to implementors. @lucacasonato WDYT?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.
Yup I agree
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.
@bartossh let's remove
total_size
then. Otherwise LGTM and we can land this PRThere 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.
@bartlomieju Curious about this. I requested the original feature and noticed that the total size of each dependency was left out in the end. I thought this a useful feature to see how big your dependencies were. Of course, you can compute this manually by running
deno info
against each dependency but that's a bit of a pain. I agree that these should be consistent, just curious about the reasoning behind leaving out the total size of dependencies.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 have implemented the change for
total_size
that @bartlomieju and @lucacasonato agreed on, removing it from vertex of a flat graph, and addedtotal_size
to module info on the top level so only total size of module will be shown.But if you think @cknight suggestion should be applied I will cherry pic previous implementation and push changes back again.
this will look like this
Thanks
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.
Circular dependencies can be really confusing, especially if they show up multiple times. Showing only the size of singular dependency is what browsers do. If there will be huge demand for "total" size we can revisit the topic, but for now I think we should go only with size of the concrete module without it's deps.
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.
Thanks @bartlomieju, makes sense