Limits length of readable identifier in default stats #16479
Conversation
|
|
|
For maintainers only:
|
|
Can you use your email (in github) for git commit, so CLA will be assigned |
Ok. I was hoping to keep my personal email out of the commit history. But I guess it's needed. I'm sure I'll have more commits, so I will include it in future commits |
|
You need to have all commits with your email (used in github, you can create two emails if you want more security and use only one for such cases), this commit can be rewritten and forcely pushed |
Alright, I'll see what I can do |
|
@alexander-akait , aside from the CLA authorization, it looks there's one other check related to code coverage integration failing. Here's the link: https://app.codecov.io/gh/webpack/webpack/pull/16479 I'm not clear on how my change impacted code coverage in the other area. Any suggestions? |
|
@trevorgithub ignore this, it is not requried (metric for us) |
Reduces the length of the readable identifier shown in webpack output for external modules, so the user is not overwhelmed with large blocks of text. See issue # 16475
trevorgithub
force-pushed
the
limit-identifier-length-in-default-stats
branch
from
2f9f1ae
to
e38b5f7
Compare
|
@alexander-akait , I squashed the commits and redid the one commit so that my email address associated with my github account is now used. Looks like the CLA check is happy now. I guess you need to do something to approve running the workflows... |
|
Thank you |
| readableIdentifier, | ||
| module | ||
| ) => { | ||
| const maxLength = 80; |
There was a problem hiding this comment.
Hoist this to the top of the file as a constant and use CONSTANT_CASE for the name. Reassigning continuously in this function adds unneeded memory pressure.
I would probably go with something like: MAX_EXTERNAL_MODULE_READABLE_IDENTIFIER_LENGTH
| const maxLength = 80; | ||
| return module instanceof ExternalModule && | ||
| readableIdentifier.length > maxLength | ||
| ? readableIdentifier.substring(0, maxLength) + "..." |
There was a problem hiding this comment.
| ? readableIdentifier.substring(0, maxLength) + "..." | |
| ? readableIdentifier.substring(0, maxLength) + "...(truncated)" |
cc @alexander-akait I think its more user friendly if its blatantly obvious that truncation is happening. The ellipsis alone is not clear enough.
There was a problem hiding this comment.
Agree, let's add (truncated)
There was a problem hiding this comment.
@TheLarkInn , @alexander-akait Thanks for your feedback on this pull request. In the original issue, 16475, Zack suggested an alternate way to achieve my goal using delegated modules. I have used his idea, and it is sufficient for my needs. As a result, I'm not going to continue with this pull request. If you think it has value outside of my needs, feel free to make the changes that you suggested. Otherwise, please feel free to close the pull request.
There was a problem hiding this comment.
@snitin315 Maybe you want to finish it? 😄
Reduces the length of the readable identifier shown in webpack output, so the user is not overwhelmed with large blocks of text. Fixes # 16475
Please see issue #16475
What kind of change does this PR introduce?
Changes the build output so that long strings are truncated (elided) to make more readable
Did you add tests for your changes?
No. Please advise where I would add tests. I looked in the project for tests of DefaultStatsFactoryPlugin.js, but I couldn't find any tests.
Does this PR introduce a breaking change?
No
What needs to be documented once your changes are merged?
I'm not sure anything needs to be documented. I suppose somewhere it could say that identifiers in the build output will be truncated after hitting a maximum length, and truncation will be indicated by the presence of some ellipsis