refactor(model)!: Simplify the `CuratedPackage` and its creation #9679

fviernau · 2024-12-30T13:20:11Z

Historically, serialized ORT results contain entries for all CuratedPackages. Back then, the list of PackageCurationResult, in particular the values of the base property had been useful to trace back from which original values any curated package had been derived.

Meanwhile, ORT result has been refactored to apply the package curations on-the-fly. So, that the serialized ORT result only contains the original, non-curated packages and the package curations separately, but no more the curated packages. So, in order to debug the derivation one can simply use break points in the function which applies the curation. So, the base property is not needed anymore.

Remove the base property, to also remove the diff() function, which is only needed for computing the base. This simplifies things, because the diff is not always straight forward to compute and make sense of. In particular when Maps are in play.

Note: It may may sense to add the original package as a separate property to CuratedPackage.

Related to: #8725.

codecov · 2024-12-30T13:33:26Z

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 68.10%. Comparing base (e7b8a3a) to head (e83a95b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...main/kotlin/licenses/DefaultLicenseInfoProvider.kt	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #9679   +/-   ##
=========================================
  Coverage     68.10%   68.10%           
  Complexity     1294     1294           
=========================================
  Files           249      249           
  Lines          8841     8841           
  Branches        922      922           
=========================================
  Hits           6021     6021           
  Misses         2432     2432           
  Partials        388      388

Flag	Coverage Δ
funTest-docker	`65.14% <ø> (ø)`
funTest-non-docker	`33.28% <0.00%> (ø)`
test-ubuntu-24.04	`35.91% <50.00%> (ø)`
test-windows-2022	`35.88% <50.00%> (ø)`

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.

sschuberth · 2025-01-02T09:07:59Z

model/src/main/kotlin/CuratedPackage.kt

@@ -20,7 +20,7 @@
 package org.ossreviewtoolkit.model

 /**
- * A [Package] including the [PackageCurationResult]s that were applied to it, in order to be able to trace back how the
+ * A [Package] including the [PackageCuration]s that were applied to it, in order to be able to trace back how the


Basically, the PR looks sensible to me. However, the PackageCurationResult class should be removed completely now as well.

Commit message:

"results containd entries"

"in particular the values"

"original values"

Comma before "had been useful"

"ORT result hasve been refactored"

To avoid multiple "So, " sentences:

"So, that the serialized ORT result" -> "This means that the serialized ORT result"

"So, the base property is not needed anymore." -> "That is why ..."

"Remove the base property, to also remove the diff() function, which
is only needed for computing the base." -> "So, remove the base property, which allows to also remove the diff() function as that was only needed to compute the base."

I'd just drop "In particular when Maps are in play.".

"It may may" -> "It may make"

Do not put "property" into backticks.

"...including the [PackageCurationData] that was applied to it..."

Not required for this PR, but looking at this class now I wonder if we should also change the metadata property of this class to contain the raw metadata of the package and add a helper function to get the package with applied curations instead.

Not required for this PR,

I thought about this too. (Also thought it could be a follow up), had basically similar idea in mind.

mnonnenmacher · 2025-01-02T14:24:39Z

model/src/main/kotlin/licenses/LicenseInfo.kt

@@ -68,7 +69,7 @@ data class ConcludedLicenseInfo(
    /**
     * The list of [PackageCurationResult]s that modified the concluded license.


Still referencing old PackageCurationResult.

mnonnenmacher · 2025-01-02T14:24:44Z

model/src/main/kotlin/licenses/LicenseInfo.kt

@@ -93,7 +94,7 @@ data class DeclaredLicenseInfo(
    /**
     * The list of [PackageCurationResult]s that modified the declared license.


Still referencing old PackageCurationResult.

mnonnenmacher · 2025-01-02T14:27:27Z

model/src/main/kotlin/CuratedPackage.kt

@@ -20,7 +20,7 @@
 package org.ossreviewtoolkit.model

 /**
- * A [Package] including the [PackageCurationResult]s that were applied to it, in order to be able to trace back how the
+ * A [Package] including the [PackageCuration]s that were applied to it, in order to be able to trace back how the


"...including the [PackageCurationData] that was applied to it..."

mnonnenmacher · 2025-01-02T14:30:44Z

model/src/main/kotlin/CuratedPackage.kt

@@ -20,7 +20,7 @@
 package org.ossreviewtoolkit.model

 /**
- * A [Package] including the [PackageCurationResult]s that were applied to it, in order to be able to trace back how the
+ * A [Package] including the [PackageCuration]s that were applied to it, in order to be able to trace back how the


Not required for this PR, but looking at this class now I wonder if we should also change the metadata property of this class to contain the raw metadata of the package and add a helper function to get the package with applied curations instead.

Historically, serialized ORT results contained entries for all `CuratedPackage`s. Back then, the list of `PackageCurationResult`, in particular the value of the `base` property, had been useful to trace back from which original value any curated package had been derived. Meanwhile, ORT results have been refactored to apply the package curations on-the-fly. This means that the serialized ORT result only contains the original, non-curated packages and the package curations separately, but no more the curated packages. So, in order to debug the derivation one can simply use break points in the function which applies the curation. That is why the `base` property is not needed anymore. So, remove the `base` property, which allows to also remove the `diff()` function as it was only needed to compute the `base`. This simplifies things, because the `diff` is not always straight forward to compute and make sense of. Note: It might make sense to add the `original` package as a separate property to `CuratedPackage`. Signed-off-by: Frank Viernau <[email protected]> Signed-off-by: Sebastian Schuberth <[email protected]>

fviernau · 2025-01-02T16:04:26Z

Thank you @sschuberth , @mnonnenmacher for moving this forward :-)

fviernau force-pushed the created-package-simplifications branch 2 times, most recently from 7036c4c to 944542c Compare December 30, 2024 13:58

fviernau mentioned this pull request Dec 30, 2024

feat(model): Add a generic labeling mechanism for packages #9674

Merged

sschuberth requested changes Jan 2, 2025

View reviewed changes

mnonnenmacher requested changes Jan 2, 2025

View reviewed changes

sschuberth force-pushed the created-package-simplifications branch from 944542c to e83a95b Compare January 2, 2025 16:02

sschuberth requested a review from mnonnenmacher January 2, 2025 16:05

sschuberth marked this pull request as ready for review January 2, 2025 16:05

sschuberth requested a review from a team as a code owner January 2, 2025 16:05

sschuberth approved these changes Jan 2, 2025

View reviewed changes

mnonnenmacher approved these changes Jan 2, 2025

View reviewed changes

sschuberth merged commit f9c7220 into main Jan 2, 2025
25 of 26 checks passed

sschuberth deleted the created-package-simplifications branch January 2, 2025 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(model)!: Simplify the `CuratedPackage` and its creation #9679

refactor(model)!: Simplify the `CuratedPackage` and its creation #9679

fviernau commented Dec 30, 2024 •

edited

Loading

codecov bot commented Dec 30, 2024 •

edited

Loading

sschuberth Jan 2, 2025

sschuberth Jan 2, 2025

mnonnenmacher Jan 2, 2025

mnonnenmacher Jan 2, 2025

fviernau Jan 2, 2025 •

edited

Loading

mnonnenmacher Jan 2, 2025

mnonnenmacher Jan 2, 2025

mnonnenmacher Jan 2, 2025

mnonnenmacher Jan 2, 2025

fviernau commented Jan 2, 2025 •

edited

Loading

		@@ -68,7 +69,7 @@ data class ConcludedLicenseInfo(
		/**
		* The list of [PackageCurationResult]s that modified the concluded license.

		@@ -93,7 +94,7 @@ data class DeclaredLicenseInfo(
		/**
		* The list of [PackageCurationResult]s that modified the declared license.

refactor(model)!: Simplify the CuratedPackage and its creation #9679

refactor(model)!: Simplify the CuratedPackage and its creation #9679

Conversation

fviernau commented Dec 30, 2024 • edited Loading

codecov bot commented Dec 30, 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 Jan 2, 2025 • 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

fviernau commented Jan 2, 2025 • edited Loading

refactor(model)!: Simplify the `CuratedPackage` and its creation #9679

refactor(model)!: Simplify the `CuratedPackage` and its creation #9679

fviernau commented Dec 30, 2024 •

edited

Loading

codecov bot commented Dec 30, 2024 •

edited

Loading

fviernau Jan 2, 2025 •

edited

Loading

fviernau commented Jan 2, 2025 •

edited

Loading