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

Inline advisor record #8865

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Inline advisor record #8865

merged 2 commits into from
Jul 11, 2024

Conversation

mnonnenmacher
Copy link
Member

The AdvisorRecord was only a holder for the map of advisor results per package. Inline it with AdvisorRun to reduce nesting in the data structures, similar to how ScanRecord was inlined with ScannerRun in 9d9a449.

@@ -333,4 +333,4 @@
return AdvisorResult(details, summary, defects, vulnerabilities)
}

private fun advisorRecordOf(vararg results: Pair<Identifier, List<AdvisorResult>>) = AdvisorRecord(results.toMap())
private fun advisorRunOf(vararg results: Pair<Identifier, List<AdvisorResult>>) = AdvisorRun.EMPTY.copy(results = results.toMap())

Check warning

Code scanning / detekt

Format signature to be single when possible, multiple lines otherwise. Warning test

Newline expected before expression body
@@ -333,4 +333,4 @@
return AdvisorResult(details, summary, defects, vulnerabilities)
}

private fun advisorRecordOf(vararg results: Pair<Identifier, List<AdvisorResult>>) = AdvisorRecord(results.toMap())
private fun advisorRunOf(vararg results: Pair<Identifier, List<AdvisorResult>>) = AdvisorRun.EMPTY.copy(results = results.toMap())

Check warning

Code scanning / detekt

Line detected, which is longer than the defined maximum line length in the code style. Warning test

Line detected, which is longer than the defined maximum line length in the code style.
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.55%. Comparing base (e4aa9e9) to head (351cf82).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8865   +/-   ##
=========================================
  Coverage     67.55%   67.55%           
  Complexity     1166     1166           
=========================================
  Files           244      244           
  Lines          7769     7769           
  Branches        864      864           
=========================================
  Hits           5248     5248           
  Misses         2165     2165           
  Partials        356      356           
Flag Coverage Δ
funTest-docker 66.78% <ø> (ø)
funTest-non-docker 33.88% <ø> (ø)
test 37.93% <ø> (ø)

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.

@mnonnenmacher mnonnenmacher force-pushed the inline-advisor-record branch from 7241575 to ca1aef4 Compare July 11, 2024 19:57
The constant will be used in later commits.

Signed-off-by: Martin Nonnenmacher <[email protected]>
The `AdvisorRecord` was only a holder for the map of advisor results per
package. Inline it with `AdvisorRun` to reduce nesting in the data
structures, similar to how `ScanRecord` was inlined with `ScannerRun` in
9d9a449.

Signed-off-by: Martin Nonnenmacher <[email protected]>
@mnonnenmacher mnonnenmacher force-pushed the inline-advisor-record branch from ca1aef4 to 351cf82 Compare July 11, 2024 20:12
@mnonnenmacher mnonnenmacher marked this pull request as ready for review July 11, 2024 20:37
Comment on lines +60 to +61
environment = Environment(),
config = AdvisorConfiguration(),
Copy link
Member

@sschuberth sschuberth Jul 11, 2024

Choose a reason for hiding this comment

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

Unrelated, but I wonder whether we should rename these kind of constants to e.g. DEFAULT instead, as the values are not really empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, but the most relevant part is empty, like the results in this case. I guess there are good arguments for both names.

@mnonnenmacher mnonnenmacher merged commit 192736f into main Jul 11, 2024
22 checks passed
@mnonnenmacher mnonnenmacher deleted the inline-advisor-record branch July 11, 2024 21:35
mnonnenmacher pushed a commit to eclipse-apoapsis/ort-server that referenced this pull request Jul 18, 2024
Align with the removal of `AdvisorRecord` [1].

[1]: oss-review-toolkit/ort#8865
github-merge-queue bot pushed a commit to eclipse-apoapsis/ort-server that referenced this pull request Jul 18, 2024
Align with the removal of `AdvisorRecord` [1].

[1]: oss-review-toolkit/ort#8865
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants