-
Notifications
You must be signed in to change notification settings - Fork 319
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
Node-related improvements #9732
Conversation
This also moves the use of `packageJsonCache` closer to its declaration for a better overview. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
While at it, rename the function parameter of `parsePackage` to make clear that the package details do not need to come from a remote. The `typealias` will be reused in an upcoming change. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
While at it, ensure to use the dedicated `projectType` instead of `managerName` as the type of project IDs. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9732 +/- ##
============================================
- Coverage 68.09% 68.00% -0.10%
+ Complexity 1294 1287 -7
============================================
Files 249 249
Lines 8846 8845 -1
Branches 923 923
============================================
- Hits 6024 6015 -9
- Misses 2433 2442 +9
+ Partials 389 388 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -137,14 +137,13 @@ internal fun parseVcsInfo(packageJson: PackageJson): VcsInfo { | |||
) | |||
} | |||
|
|||
typealias PackageDetailsFunction = (packageName: String) -> PackageJson? |
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.
Function
is so generic, how about PackageDetailsProvider
or GetPackageDetails
?
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 had "Provider" initially, but compared to our other providers, like PackageCurationProvider
, this is way more lightweight, which is why I went for "Function".
Also I didn't want to call it "GetPackageDetails", as that name IMO better fits for the variable / parameter name in line 146, and I didn't want to call the parameter and type basically the same, just with different capitalization.
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 think it would not be bad to call it the same. But it's also not so important.
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.
Hmm, as somewhat of a compromise, would you like GetPackageDetailsFun
better?
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.
Let's eventually change this later on.
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.
See #9738.
plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt
Outdated
Show resolved
Hide resolved
Merging despite the unrelated |
Please have a look at the individual commit messages for the details.