Pub: Improve the code for the lockfile parsing #9046

fviernau · 2024-08-29T11:10:15Z

Simplify migrating to KxS later on. See individual commits.

codecov · 2024-08-29T11:23:36Z

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.01%. Comparing base (5d11ab0) to head (aefbf0b).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...nagers/pub/src/main/kotlin/utils/PubCacheReader.kt	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9046      +/-   ##
============================================
- Coverage     68.02%   68.01%   -0.02%     
- Complexity     1158     1161       +3     
============================================
  Files           241      241              
  Lines          7691     7691              
  Branches        867      871       +4     
============================================
- Hits           5232     5231       -1     
  Misses         2103     2103              
- Partials        356      357       +1

Flag	Coverage Δ
funTest-docker	`67.22% <62.50%> (-0.18%)`	⬇️
funTest-non-docker	`33.92% <ø> (ø)`
test	`37.91% <87.50%> (-0.02%)`	⬇️

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.

plugins/package-managers/pub/src/main/kotlin/Lockfile.kt

sschuberth · 2024-08-29T14:43:14Z

plugins/package-managers/pub/src/main/kotlin/Lockfile.kt

+
+@JsonIgnoreProperties(ignoreUnknown = true)
+internal data class Lockfile(
+    val packages: Map<String, PackageInfo> = emptyMap()


Upstream seems to call the values PackageId. Should we align?

Hm, on the one hand it's good to align to keep things simple. However,
we also usually name the variables (e.g. packageInfo) after the class (e.g. PackageInfo).

Renaming these as well to packageId IMO creates more confusion, as we have different terminology ffor packageId within ORT. I'd keep it for now and re-consider it later, when more code clean-up is done. Are you fine with that?

Yes, I agree that the upstream name is a bit awkward, so let's keep ours, at least for now.

plugins/package-managers/pub/src/main/kotlin/Lockfile.kt

sschuberth · 2024-08-29T15:02:01Z

plugins/package-managers/pub/src/main/kotlin/Lockfile.kt

+        val name: String? = null,
+        val url: String? = null,
+        val path: String? = null,
+        @JsonProperty("resolved-ref")
+        val resolvedRef: String? = null,
+        val relative: Boolean? = null,
+        val sha256: String? = null


Similar for these properties, it's hard to believe they're all optional. Maybe try the opposite way and make them all mandatory, see where tests fail, and adjust only if necessary?

Would it make sense to do this in a separate commit on top, to keep this commit more like a non-functional transformation? (Some of these are certainly nullable)

plugins/package-managers/pub/src/main/kotlin/Pub.kt

sschuberth · 2024-08-29T15:06:55Z

plugins/package-managers/pub/src/main/kotlin/Pub.kt

-                                }
-
-                                VcsInfo.EMPTY
+        lockfile.packages.forEach { (packageName, pkgInfoFromLockfile) ->


The outer listOf("packages"/*, "packages-dev"*/).forEach { should be removed in a preceding commit as it's unrelated to this change and deserves a dedicated rationale. That also reduces nesting already beforehand so that the relevant changes here become more obvious.

Simplify an upcoming change. Signed-off-by: Frank Viernau <[email protected]>

The comment does not help much. Remove it to prepare for an upcoming change. Signed-off-by: Frank Viernau <[email protected]>

Signed-off-by: Frank Viernau <[email protected]>

Simplify an upcoming change. Signed-off-by: Frank Viernau <[email protected]>

sschuberth · 2024-08-30T09:40:48Z

plugins/package-managers/pub/src/main/kotlin/Lockfile.kt

+internal fun parseLockfile(lockfile: File) = yamlMapper.readValue<Lockfile>(lockfile)
+
+/**
+ * See  https://github.com/dart-lang/pub/blob/d86e3c979a3889fed61b68dae9f9156d0891704d/lib/src/lock_file.dart#L18.


Nit: Double space after "See".

Ping, just in case you overlooked this @fviernau.

sschuberth · 2024-08-30T09:50:30Z

plugins/package-managers/pub/src/test/kotlin/utils/PubCacheReaderTest.kt

@@ -59,7 +59,6 @@ class PubCacheReaderTest : WordSpec({
                        {
                            "dependency": "direct main",
                            "description": {
-                                "ref": "master",


Is it really much simpler to remove this compared to adding ref to PackageInfo.Description for completeness? I somehow liked to be able to get an overview of unresolved vs resolved ref even if it's unused currently.

Or actually, wouldn't the code just with without this change and without adding ref to PackageInfo.Description as you have @JsonIgnoreProperties(ignoreUnknown = true)?

Is it really much simpler to remove this compared to adding ref to PackageInfo.Description for completeness?

It has to be removed anyway, because from the following commit onwards, the test no more operates on JSON input.

I somehow liked to be able to get an overview of unresolved vs resolved ref even if it's unused currently.
...
Or actually, wouldn't the code just with without this change and without adding ref to PackageInfo.Description as you have @JsonIgnoreProperties(ignoreUnknown = true)?

Yes, I've prefer to use ignore unkown and not add unused properties.

Yes, I've prefer to use ignore unkown and not add unused properties.

Just to share my thinking here: That makes it a bit hard to get an overview over the features / metadata a package manager provides in order to maybe implement new features in the future.

But I'm ok with keeping this as it is in the PR now.

Signed-off-by: Frank Viernau <[email protected]>

fviernau requested a review from a team as a code owner August 29, 2024 11:10

sschuberth requested changes Aug 29, 2024

View reviewed changes

fviernau force-pushed the pub-improve-lockfile-parsing branch 2 times, most recently from 4d05924 to 1d62766 Compare August 30, 2024 08:21

fviernau added 5 commits August 30, 2024 11:11

test(pub): Remove an unhandled property

9958836

Simplify an upcoming change. Signed-off-by: Frank Viernau <[email protected]>

test(pub): Remove an unnecessary code comment

04c4a77

The comment does not help much. Remove it to prepare for an upcoming change. Signed-off-by: Frank Viernau <[email protected]>

test(pub): Consistently use reader

a3b1622

Signed-off-by: Frank Viernau <[email protected]>

test(pub): Add a () to a test case name

f4a96ba

Signed-off-by: Frank Viernau <[email protected]>

refactor(pub): Remove an unnecessary for loop and comment

c452dab

Simplify an upcoming change. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the pub-improve-lockfile-parsing branch from 1d62766 to 00856e6 Compare August 30, 2024 09:35

fviernau requested a review from sschuberth August 30, 2024 09:36

sschuberth requested changes Aug 30, 2024

View reviewed changes

fviernau added 2 commits August 30, 2024 12:12

refactor(pub): Use a data class for parsing the lockfile

4ea9dbf

Signed-off-by: Frank Viernau <[email protected]>

refactor(pub): Use a shorter name for pkgInfoFromLockfile

aefbf0b

Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the pub-improve-lockfile-parsing branch from 00856e6 to aefbf0b Compare August 30, 2024 10:12

fviernau requested a review from sschuberth August 30, 2024 10:12

fviernau enabled auto-merge (rebase) August 30, 2024 10:13

sschuberth approved these changes Aug 30, 2024

View reviewed changes

fviernau merged commit a45bd86 into main Aug 30, 2024
23 checks passed

fviernau deleted the pub-improve-lockfile-parsing branch August 30, 2024 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pub: Improve the code for the lockfile parsing #9046

Pub: Improve the code for the lockfile parsing #9046

fviernau commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

sschuberth Aug 29, 2024

fviernau Aug 30, 2024

sschuberth Aug 30, 2024

sschuberth Aug 29, 2024

fviernau Aug 30, 2024 •

edited

Loading

sschuberth Aug 29, 2024

sschuberth Aug 30, 2024

sschuberth Aug 30, 2024

sschuberth Aug 30, 2024

fviernau Aug 30, 2024

sschuberth Aug 30, 2024

Pub: Improve the code for the lockfile parsing #9046

Pub: Improve the code for the lockfile parsing #9046

Conversation

fviernau commented Aug 29, 2024 • edited Loading

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

fviernau Aug 30, 2024 •

edited

Loading