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

Drop unnecessary filter on artifacts #3701

Merged

Conversation

alexarchambault
Copy link
Contributor

@alexarchambault alexarchambault commented Oct 9, 2024

Artifacts should already be filtered by coursier.Artifacts (via the default classifiers and artifact types stuff), no need to filter them further

Artifacts should already be filtered by coursier.Artifacts (via
classifiers and artifact types stuff)
@alexarchambault alexarchambault marked this pull request as ready for review October 9, 2024 15:51
@@ -104,7 +104,7 @@ trait CoursierSupport {
Agg.from(
res.files
.map(os.Path(_))
.filter(path => path.ext == "jar" && resolveFilter(path))
.filter(resolveFilter)
Copy link
Member

Choose a reason for hiding this comment

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

To preserve previous behavior, you should change the resolveFilter default to contain the path.ext == "jar" match, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Precisely, that's not needed. Coursier already filters artifacts (by default, it keeps these types, which are all fine to be put in a class path). My guess is this filter was necessary when you were using the coursier 1.x API, which didn't filter artifacts as much out-of-the-box.

The PR here should be considered to be a follow-up of #3513

Copy link
Contributor Author

@alexarchambault alexarchambault Oct 10, 2024

Choose a reason for hiding this comment

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

Also, that change is necessary for #3702 and #3703. For #3702, we want klib artifacts to be fetched out-of-the-box, so we must accept *.klib files. For #3703, if users add a fancy type in JavaModule.artifactTypes, we want it to be put in the class path too, without requiring more changes by users.

lihaoyi pushed a commit that referenced this pull request Oct 10, 2024
Fixes #3695 along with
#3701 (the two PRs are needed)
@lihaoyi lihaoyi merged commit 6b83169 into com-lihaoyi:main Oct 10, 2024
24 checks passed
@alexarchambault alexarchambault deleted the no-superfluous-artifact-filtering branch October 10, 2024 09:48
@lefou lefou added this to the 0.12.0 milestone Oct 12, 2024
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