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

npm: Prepare to re-use parsePackage() from outside of Npm #9323

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

fviernau
Copy link
Member

@fviernau fviernau commented Oct 23, 2024

Move all logger dependencies from parsePackage() to the callers, to avoid the need for passing the logger when moving this function outside of Npm. And finally perform the move. This prepares for removing the inheritance in node package managers.

Part of #9261.

@fviernau fviernau requested a review from a team as a code owner October 23, 2024 09:35
@fviernau fviernau force-pushed the yarn-simplifcations branch 2 times, most recently from 191e933 to fb55fa1 Compare October 23, 2024 09:43
Copy link

codecov bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.45%. Comparing base (d2cfce1) to head (33aeb8d).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #9323   +/-   ##
=========================================
  Coverage     67.45%   67.45%           
  Complexity     1200     1200           
=========================================
  Files           241      241           
  Lines          8498     8500    +2     
  Branches        904      904           
=========================================
+ Hits           5732     5734    +2     
  Misses         2402     2402           
  Partials        364      364           
Flag Coverage Δ
funTest-docker 60.59% <100.00%> (+0.04%) ⬆️
funTest-non-docker 33.60% <ø> (ø)
test 37.47% <66.66%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fviernau fviernau force-pushed the yarn-simplifcations branch from fb55fa1 to 8409ba2 Compare October 23, 2024 10:06
@fviernau fviernau marked this pull request as draft October 23, 2024 10:54
@fviernau fviernau force-pushed the yarn-simplifcations branch from 8409ba2 to b8b7c75 Compare October 23, 2024 11:22
@fviernau fviernau marked this pull request as ready for review October 23, 2024 11:23
@fviernau fviernau changed the title node: Some simplifications and improvements npm: Prepare to re-use parsePackage() from outside of Npm Oct 23, 2024
Copy link
Member

@sschuberth sschuberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I stopped reviewing at the second commit as I believe it needs to be reworked, and all further changes build upon it.

plugins/package-managers/node/src/main/kotlin/Yarn.kt Outdated Show resolved Hide resolved
@fviernau
Copy link
Member Author

Note that I stopped reviewing at the second commit as I believe it needs to be reworked, and all further changes build upon it.

I've commented on your comments. It'd be great to get some more hint why / in what direction it shall be re-worked.

@fviernau fviernau requested a review from sschuberth October 23, 2024 20:05
@fviernau fviernau force-pushed the yarn-simplifcations branch 2 times, most recently from e7139a9 to 06f5aa5 Compare October 24, 2024 13:38
@fviernau fviernau force-pushed the yarn-simplifcations branch from 06f5aa5 to 0744817 Compare October 24, 2024 16:02
@fviernau fviernau requested a review from sschuberth October 24, 2024 16:03
This way `getRemotePackageDetails()` doesn't throw anymore which
enables further simplifications. Also the code gets aligned with the
code in `Yarn`.

Signed-off-by: Frank Viernau <[email protected]>
All implementations of `getRemotePackageDetails()` by now return `null`
in case of a failure instead of throwing. Remove `runCatching()`, because
it is not needed anymore.

Signed-off-by: Frank Viernau <[email protected]>
Prepare for re-using this function from current child classes of `Npm`,
once they get refactored to not inherit from `Npm` anymore.

Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau force-pushed the yarn-simplifcations branch from 0744817 to 33aeb8d Compare October 24, 2024 16:03
@fviernau fviernau merged commit 1394274 into main Oct 24, 2024
23 checks passed
@fviernau fviernau deleted the yarn-simplifcations branch October 24, 2024 20:34
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.

2 participants