Skip to content

Conversation

@mnonnenmacher
Copy link
Member

Move the function to determine the enabled package managers to the public API of the analyzer to make it available when programmatically using ORT.

@mnonnenmacher mnonnenmacher requested a review from a team as a code owner June 22, 2023 10:59
@mnonnenmacher mnonnenmacher enabled auto-merge (rebase) June 22, 2023 11:00
* [AnalyzerConfiguration.disabledPackageManagers] configuration properties and the
* [default][PackageManagerFactory.isEnabledByDefault] of the [PackageManager]s.
*/
fun AnalyzerConfiguration.determineEnabledPackageManagers(): Set<PackageManagerFactory> {
Copy link
Member

Choose a reason for hiding this comment

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

@fviernau had the idea recently to not group functions by their type anymore, i.e. not put extensions functions into Extensions.kt, but by functionality. I basically think that's a good idea. So instead of introducing a new Extensions.kt, should we maybe move the function to e.g. analyzer/src/main/kotlin/Analyzer.kt?

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'm not sure about that, maybe we should discuss in the next dev meeting if we want to define some rules here. To me Extensions.kt fits well in this case, because it's an API extension to the AnalyzerConfiguration class. Also, if moved it should probably go to PackageManager.kt instead of Analyzer.kt because that's the dependency of the function, but I'm also not a fan of putting too much functionality in a single file because it's against separation of concerns. Actually, I would also be for moving the parseAuthorString function out of PackageManager.kt for that reason.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then let's stick to our current conventions for now and bring this up in the next Kotlin Developer Meeting.

Copy link
Member

Choose a reason for hiding this comment

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

But there are unused imports 😉

sschuberth
sschuberth previously approved these changes Jun 22, 2023
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (d34668c) 64.53% compared to head (c339742) 64.53%.

Additional details and impacted files
@@ Coverage Diff @@ ## main #7180 +/- ## ========================================= Coverage 64.53% 64.53% Complexity 1972 1972 ========================================= Files 334 334 Lines 16725 16725 Branches 2399 2399 ========================================= Hits 10793 10793 Misses 4885 4885 Partials 1047 1047 
Flag Coverage Δ
funTest-non-docker 27.99% <ø> (ø)
test 40.59% <ø> (ø)

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

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

Move the function to determine the enabled package managers to the public API of the analyzer to make it available when programmatically using ORT. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
@mnonnenmacher mnonnenmacher force-pushed the determine-enabled-package-managers-public branch from 6decdca to c339742 Compare June 22, 2023 13:05
@mnonnenmacher mnonnenmacher requested a review from sschuberth June 22, 2023 13:06
@mnonnenmacher mnonnenmacher merged commit 7aed5f1 into main Jun 22, 2023
@mnonnenmacher mnonnenmacher deleted the determine-enabled-package-managers-public branch June 22, 2023 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants