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

Snippet Choice #8257

Conversation

nnobelis
Copy link
Member

This pull request introduces the Snippet Choice feature to ORT.

Please have a look at the documentation in website/docs/configuration/snippet-choice.md.

@nnobelis nnobelis force-pushed the nnobelis/snippet_choice_step_1 branch 9 times, most recently from 0e0d89b to 1929262 Compare February 15, 2024 15:07
@nnobelis nnobelis marked this pull request as ready for review February 15, 2024 15:31
@nnobelis nnobelis requested review from a team as code owners February 15, 2024 15:31
@nnobelis
Copy link
Member Author

According to @mnonnenmacher , this is normal the markdown-links test is failing as the link is pointing to a file that is added in current PR, and not present in main yet. There is no workaround, it can be ignored.

model/src/main/kotlin/config/PackageSnippetChoice.kt Outdated Show resolved Hide resolved
cli/src/funTest/assets/git-repo-expected-output.yml Outdated Show resolved Hide resolved
model/src/main/kotlin/config/PackageSnippetChoice.kt Outdated Show resolved Hide resolved
model/src/main/kotlin/config/PackageSnippetChoice.kt Outdated Show resolved Hide resolved
model/src/main/kotlin/config/RepositoryConfiguration.kt Outdated Show resolved Hide resolved
model/src/main/kotlin/config/SnippetChoice.kt Outdated Show resolved Hide resolved
model/src/main/kotlin/config/SnippetChoiceReason.kt Outdated Show resolved Hide resolved
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
scanner/src/main/kotlin/ScanContext.kt Outdated Show resolved Hide resolved
@nnobelis nnobelis force-pushed the nnobelis/snippet_choice_step_1 branch 2 times, most recently from ff6b70e to 3d163d9 Compare February 16, 2024 07:10
@nnobelis nnobelis requested a review from sschuberth February 16, 2024 07:30
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
@@ -0,0 +1,187 @@
The snippets are short pieces of code coming from difference public sources such as GitHub, GitLab, Stackoverflow, ...

Some product such as ScanOSS and FossID scrape those public sources and build a Knowledge Base of snippets, classifying
Copy link
Member

Choose a reason for hiding this comment

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

"Some product such as" -> "So-called snippet scanners like"

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't capitalize "Knowledge Base" or "Author, Version and License" in the next sentence.

Copy link
Member

Choose a reason for hiding this comment

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

Note: I'm pausing review here.

@nnobelis nnobelis force-pushed the nnobelis/snippet_choice_step_1 branch 3 times, most recently from 5ef4693 to a92b14b Compare February 19, 2024 06:36
@nnobelis nnobelis requested a review from sschuberth February 19, 2024 06:38
@sschuberth sschuberth requested a review from a team February 19, 2024 07:34
@nnobelis nnobelis force-pushed the nnobelis/snippet_choice_step_1 branch from a92b14b to bfb6cbc Compare February 19, 2024 08:02
@nnobelis nnobelis requested a review from sschuberth February 19, 2024 08:03
/**
* The URL of the [RepositoryProvenance] the snippet choice applies to.
*/
data class Provenance(val url: String)
Copy link
Member

Choose a reason for hiding this comment

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

Can you give this class another name to avoid a name conflict if some code has to use this and the other Provenance class? I don't have a good idea, maybe TargetProvenance?

Copy link
Member

@sschuberth sschuberth Feb 22, 2024

Choose a reason for hiding this comment

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

Or also move to a snippet sub-package to disambiguate the FQN?

Copy link
Member

Choose a reason for hiding this comment

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

@sschuberth Did you mean to only move or to rename and move? I guess I can live with only moving it like it was done now.

Copy link
Member

Choose a reason for hiding this comment

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

I was meaning to only move.

model/src/main/kotlin/config/snippet/SnippetChoice.kt Outdated Show resolved Hide resolved
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
website/docs/configuration/snippet-choice.md Outdated Show resolved Hide resolved
scanner/src/main/kotlin/ScanContext.kt Show resolved Hide resolved
@nnobelis nnobelis force-pushed the nnobelis/snippet_choice_step_1 branch from bfb6cbc to d909214 Compare February 23, 2024 07:11
mnonnenmacher
mnonnenmacher previously approved these changes Feb 23, 2024
@mnonnenmacher
Copy link
Member

@sschuberth LGTM, do you want to have another look?

@sschuberth sschuberth dismissed their stale review February 23, 2024 13:01

I won't have the time to finish the docs revewi soon.

@sschuberth
Copy link
Member

@sschuberth LGTM, do you want to have another look?

I guess I would probably still have plenty of wording-nits for snippet-choice.md, but I currently do not have time to look at it again thoroughly, so I do not want to block this. I skipped over the code, and from that perspective it looks ok to me now.

@mnonnenmacher
Copy link
Member

@sschuberth LGTM, do you want to have another look?

I guess I would probably still have plenty of wording-nits for snippet-choice.md, but I currently do not have time to look at it again thoroughly, so I do not want to block this. I skipped over the code, and from that perspective it looks ok to me now.

I think the docs don't have to be perfect from the beginning, we can still improve it later.

@mnonnenmacher
Copy link
Member

@nnobelis The funTest error is not related to your change, but for the test job you have to fix the RepositoryConfigurationTest.

By adding the snippet choices configuration to the scanner context, the
snippet scanner implementations such as ScanOSS and FossID can be extended
to take into account this configuration.
This will be done in a future commit.

Signed-off-by: Nicolas Nobelis <[email protected]>
@mnonnenmacher mnonnenmacher merged commit 4e0b6a1 into oss-review-toolkit:main Feb 23, 2024
17 of 20 checks passed
@mnonnenmacher mnonnenmacher deleted the nnobelis/snippet_choice_step_1 branch February 23, 2024 15:37
@sschuberth
Copy link
Member

sschuberth commented Feb 23, 2024

I now realized model/src/main/kotlin/config/SnippetChoices.kt is not in the snippet package like the rest is. Is this an oversight?

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.

3 participants