-
Notifications
You must be signed in to change notification settings - Fork 313
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
Yarn2 simplifications #9486
Yarn2 simplifications #9486
Conversation
Improve readability. Signed-off-by: Frank Viernau <[email protected]>
The parsing should be fast enough, so that the log output is unneeded. Signed-off-by: Frank Viernau <[email protected]>
Signed-off-by: Frank Viernau <[email protected]>
Improve readability. Signed-off-by: Frank Viernau <[email protected]>
Improve readability and simplify an upcoming change. Signed-off-by: Frank Viernau <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9486 +/- ##
============================================
+ Coverage 67.93% 67.94% +0.01%
- Complexity 1289 1290 +1
============================================
Files 249 249
Lines 8794 8798 +4
Branches 913 912 -1
============================================
+ Hits 5974 5978 +4
Misses 2434 2434
Partials 386 386
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
7e0c22e
to
bd92e03
Compare
@@ -642,6 +642,8 @@ private data class PackageHeader( | |||
|
|||
private val PackageHeader.moduleId: String get() = "$rawName@${version.cleanVersionString()}" | |||
|
|||
private val PackageHeader.isProject: Boolean get() = type == "workspace" |
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.
Nit: Maybe put before moduleId
to maintain alphabetical sorting of extension functions?
@@ -443,7 +443,7 @@ class Yarn2( | |||
vcsFromPackage = vcsFromPackage.merge(details.vcsFromDownloadUrl) | |||
} | |||
|
|||
val id = Identifier("NPM", namespace, name, details.version) | |||
val id = Identifier("NPM", namespace, name, version) |
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.
Commit message: It's a bit odd to refer to an upcoming change that's not part of this PR. Also, just from looking at the diff it's not clear why details.
can just be omitted, i.e. why details.version
and version
seem to refer to the same value.
Actually, as version
is defined as
val version = header.version.cleanVersionString()
and used as
val details = packagesDetails["${header.rawName}@$version"]
I'd argue that version
should only be used to create the key for the lookup in packagesDetails
, and otherwise details.version
should exclusively be used as the package version.
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 believe the change makes sense and is a valid simplification, because of this:
details
is aPackageJson
instance and it is requested with themoduleId
which uses the cleaned version obtained fromcleanVersionString()
(viewyarn npm view
).- The
PackageJson.version
returned by thatyarn npm view
always is identical with theversion
requirested,
which implies thedetails.version
andversion
always is identical.
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.
2. The
PackageJson.version
returned by thatyarn npm view
always is identical with theversion
requirested
Would you have a(n API) reference for that?
Anyway, would it make sense to postpone this commit to the PR that actually benefits from it? Right now I just don't see why replacing details.version
with version
is an improvement besides a few characters saved. To me, this is semantically mixing up "the version used for requesting details" with "the version returned as part of details" even if they may be the same.
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.
Ok, I've dropped it.
Improve readability and simplify an upcoming change. Signed-off-by: Frank Viernau <[email protected]>
It is not necessary to take `PackageHeader`s. Signed-off-by: Frank Viernau <[email protected]>
d71ff2d
to
0b4edd7
Compare
This PR is only a starter for a series of PRs to make the code simpler and easier to read.
This are preparations which occurred when trying to re-use the
parsePackage()
andparseProject()
functions used for the other three node managers, but further preparations will be needed..