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

Write out checksums for SPDX binary / source packages #7066

Merged
merged 11 commits into from
Jun 2, 2023

Conversation

sschuberth
Copy link
Member

See 1 for the background discussion.

@sschuberth sschuberth requested a review from a team as a code owner May 30, 2023 20:14
@sschuberth sschuberth marked this pull request as draft May 30, 2023 20:46
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (65c6612) 64.56% compared to head (a59503d) 64.55%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7066      +/-   ##
============================================
- Coverage     64.56%   64.55%   -0.01%     
  Complexity     1969     1969              
============================================
  Files           331      331              
  Lines         16617    16619       +2     
  Branches       2373     2374       +1     
============================================
  Hits          10729    10729              
- Misses         4844     4845       +1     
- Partials       1044     1045       +1     
Flag Coverage Δ
funTest-non-docker 29.58% <ø> (+0.57%) ⬆️
test 40.32% <0.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
utils/spdx/src/main/kotlin/model/SpdxPackage.kt 78.46% <0.00%> (-2.50%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sschuberth sschuberth force-pushed the spdx-package-checksum branch 2 times, most recently from 0e5fb20 to 41b8174 Compare May 31, 2023 08:35
@sschuberth sschuberth force-pushed the spdx-package-checksum branch 3 times, most recently from b9fa63c to a8f3bf1 Compare May 31, 2023 09:43
@sschuberth sschuberth marked this pull request as ready for review May 31, 2023 10:18
@sschuberth sschuberth changed the title feat(spdx-reporter): Write out the checksum for the binary package Write out checksums for SPDX binary / source packages May 31, 2023
@fviernau fviernau requested a review from tsteenbe June 1, 2023 07:26
…ction

This fits better with the other functions in this file.

Signed-off-by: Sebastian Schuberth <[email protected]>
The suffix will be used in an upcoming change. While at it, also default
the infix to "Package".

Signed-off-by: Sebastian Schuberth <[email protected]>
See [1] for the background discussion.

[1]: #7064

Signed-off-by: Sebastian Schuberth <[email protected]>
Align with the remaining functions in this file an specifiy the return
type explicitly.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth requested a review from fviernau June 2, 2023 08:07
@sschuberth sschuberth enabled auto-merge (rebase) June 2, 2023 08:21
@sschuberth sschuberth disabled auto-merge June 2, 2023 12:08
@@ -0,0 +1,171 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Now, as we have move to such a fine grained module structure - to me it makes even less sense to determine a location for a function based on whether it's an extension function or not and then as second priority the alphabetical order.

IMO cohesion is more important: e.g. if a non-extension utility function is closely related to an extension function, then IMO it should be located nearby.

What do you think? In particular about the alternatives to

  1. Keep the functions where they currently are
  2. Have a utils class where when can also put non-extension functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

to me it makes even less sense to determine a location for a function based on whether it's an extension function or not [...] if a non-extension utility function is closely related to an extension function, then IMO it should be located nearby

I'm open to the idea to group functions, extensions or not, by purpose instead of syntax, although having an Extensions.kt seems to be pretty common in Kotlin projects. Anyway, I guess we should do this consistently throughout the code base in a separate PR then.

* Convert an ORT [Hash] to an [SpdxChecksum], or return null if a conversion is not possible.
*/
private fun Hash.toSpdxChecksum(): SpdxChecksum? =
SpdxChecksum.Algorithm.values().find { it.name in algorithm.aliases }?.let {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to add a code comment like: // Assume that algorithm aliases in ORT and SPDX match.?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do it in a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #7085.

@sschuberth
Copy link
Member Author

Merging despite the unrelated GoModFunTest failure.

@sschuberth sschuberth merged commit f646809 into main Jun 2, 2023
@sschuberth sschuberth deleted the spdx-package-checksum branch June 2, 2023 18:29
@sschuberth sschuberth added the release notes Changes that should be mentioned in release notes label Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes Changes that should be mentioned in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants