-
Notifications
You must be signed in to change notification settings - Fork 314
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
Further NPM CLI issue adjustments #8193
Conversation
Signed-off-by: Sebastian Schuberth <[email protected]>
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.
LGTM, I prefer to have another approval as I'm not certain about using "NPM CLI" as Issue.source
. Maybe @mnonnenmacher ?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8193 +/- ##
=========================================
Coverage 67.16% 67.16%
Complexity 2049 2049
=========================================
Files 357 357
Lines 17158 17158
Branches 2461 2461
=========================================
Hits 11525 11525
Misses 4611 4611
Partials 1022 1022
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
074f522
to
36e397d
Compare
I thought about this and am not sure if I like the approach. The reason is that for a user who does not know the background it's not clear why some issues use "NPM" and others "NPM CLI" as source. Also, if you want to filter NPM issues (e.g. in the workbench) you now have potentially two different values for the source to filter by. |
Ok, let me just drop the second commit then to simplify the decision making here. |
36e397d
to
e9e2cd5
Compare
Please have a look at the individual commit messages for the details.
The second commits is just a proposal, I'm fine to also drop it again.