Skip to content

Conversation

fviernau
Copy link
Member

@fviernau fviernau commented Dec 14, 2023

This PR first of all ensures that the evaluator and reporter and list-copyrights commands add all package configurations to the OrtResult before passing it around, and applies resulting simplifications.

This prepares for changing OrtResult.getIssues() to filter the issues by path excludes in a following PR.
As a I nice side effect, package configurations are now only resolved once per CLI command run instead of multiple times.

Part of #7921.

@fviernau fviernau force-pushed the reporter-input-rm-pkg-config-provider branch from 5555871 to f296f2a Compare December 14, 2023 12:35
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (fad0008) 67.16% compared to head (3e54e5e) 67.16%.

Files Patch % Lines
.../src/main/kotlin/commands/ListCopyrightsCommand.kt 0.00% 3 Missing ⚠️
model/src/main/kotlin/utils/OrtResultExtensions.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8039   +/-   ##
=========================================
  Coverage     67.16%   67.16%           
+ Complexity     2054     2052    -2     
=========================================
  Files           358      358           
  Lines         17171    17166    -5     
  Branches       2462     2462           
=========================================
- Hits          11533    11530    -3     
+ Misses         4615     4613    -2     
  Partials       1023     1023           
Flag Coverage Δ
funTest-non-docker 33.96% <0.00%> (+0.02%) ⬆️
test 37.01% <33.33%> (-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.

@fviernau fviernau added the on hold Pull requests that cannot currently be merged label Dec 27, 2023
@fviernau fviernau force-pushed the reporter-input-rm-pkg-config-provider branch 3 times, most recently from bdb13a2 to 87cd546 Compare January 16, 2024 15:59
@fviernau fviernau changed the title Filter scan issues by path excludes to eliminate irrelevant issues Remove ReporterInput.packageConfigurationProvider Jan 16, 2024
@fviernau fviernau added reporter About the reporter tool cli-helper About the Command Line Interface helper and removed on hold Pull requests that cannot currently be merged labels Jan 16, 2024
@fviernau fviernau marked this pull request as ready for review January 16, 2024 16:03
@fviernau fviernau requested a review from a team as a code owner January 16, 2024 16:03
@sschuberth
Copy link
Member

FYI, the SPM test update is part of my #8119.

@fviernau fviernau force-pushed the reporter-input-rm-pkg-config-provider branch from 87cd546 to c116b18 Compare January 16, 2024 16:32
@fviernau fviernau enabled auto-merge (rebase) January 16, 2024 16:33
@fviernau fviernau force-pushed the reporter-input-rm-pkg-config-provider branch from c116b18 to 7a9fb30 Compare January 16, 2024 16:38
@fviernau fviernau requested a review from sschuberth January 16, 2024 16:38
Prepare for making use of it in a following change. While at it make
`OrtResult` implement `PackageConfigurationProvider` to document that
the added API is identical with the interface.

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
Include the package configurations into the `OrtResult` and adjust
`processAllCopyrightStatements()` to consume them from the `OrtResult`.
This prepares for only passing on `OrtResult`s instead of `OrtResult`
`PackageConfigurationProvider` tuples.

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
Retrieve all package configurations from the provider earlier and only
once instead of twice, and integrate them into the input `OrtResult`
before passing the `OrtResult` to the evaluator. This obviously makes
it unnecessary to pass the `PackageConfigurationProvider` to the
evaluator and prepares for removing the corresponding parameters.

Until then, just pass the `OrtResult` as package configuration
provider, to avoid resolving the package configurations again.

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
Retrieve all package configurations from the provider earlier, and
integrate them into the `OrtResult` before passing the `OrtResult` to
the reporters. This obviously makes it unnecessary to pass the
`PackageConfigurationProvider` to the reporters and prepares for
removing the corresponding property from `ReporterInput`, so that it
does not get passed around anymore.

Until then, just pass the `OrtResult` as package configuration
provider, to avoid resolving the package configurations again.

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
All callers have been changed to include the package configuration from
the provider into the `OrtResult` before passing it to that
constructor. So, the parameter `packageConfigurationProvider` has become
redundant and can be removed.

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
The reporter command has been changed to include all package
configurations into the provided `OrtResult`. So,
`ReporterInput.packageConfigurationProvider` has become redundant. Stop
using it to prepare for its removal.

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
The reporter command ensures that all package configurations are
contained in the ORT result. So, the resolution provider is not
necessary anymore.

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
@fviernau fviernau force-pushed the reporter-input-rm-pkg-config-provider branch from 7a9fb30 to 3e54e5e Compare January 16, 2024 16:54
@fviernau fviernau merged commit 3042e35 into main Jan 16, 2024
@fviernau fviernau deleted the reporter-input-rm-pkg-config-provider branch January 16, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli-helper About the Command Line Interface helper reporter About the reporter tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants