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

Ignore non-standard repositories #225

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

alexarchambault
Copy link
Contributor

@alexarchambault alexarchambault commented Jan 20, 2025

This PR tries to address an issue seen in com-lihaoyi/mill#4353. It does so by ignoring non-Maven or Ivy repositories, when giving a repository list to scalafix.

com-lihaoyi/mill#4145 introduced such a repository in the default repo list in Mill, to expose Mill's internal modules to coursier. This repository can't be straightforwardly converted to a coursierapi.Repository. Hence the error we're seeing in com-lihaoyi/mill#4353. This repository can be ignored fine by scalafix (scalafix wouldn't use it to fetch scalafix artifacts anyway).

The changes here imply other such repositories will be silently ignored by scalafix. Alternatively, we could try to detect and ignore only Mill's internal repo, and keep failing on the others.

@alexarchambault
Copy link
Contributor Author

alexarchambault commented Jan 20, 2025

We could also add a scalafixRepositories task, filtering repositories upfront. (Edit: just did that)

Allowing users to filter out repositories for scalafix
* issue for non-Maven or Ivy repositories. Overriding this task and filtering those repositories out allows to work
* around that.
*/
def scalafixRepositories: Task[Seq[Repository]] = Task.Anon {
Copy link
Owner

Choose a reason for hiding this comment

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

What's the difference between Task.Anon and Task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The former doesn't serialize and cache its result, while the latter does. CoursierModule#repositoriesTask relies on that for example, as all coursier.Repository instances cannot all be serialized to JSON (coursier.Repository is an "open" abstract class, we don't know all its implementations beforehand). We need to do the same here, as coursier.Repository is involved.

def scalafixRepositories: Task[Seq[Repository]] = Task.Anon {
repositoriesTask().filter {
case repo if repo.getClass.getName == "mill.scalalib.JavaModule$InternalRepo" =>
// Change to this when bumping to Mill > 0.12.5:
Copy link
Owner

@joan38 joan38 Jan 20, 2025

Choose a reason for hiding this comment

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

Will we also need the current case statement for backcompatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that can be changed only when bumping to 0.13

false
case _ => true
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

nit: Do you mind adding a space after this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@joan38 joan38 enabled auto-merge (rebase) January 21, 2025 00:34
@joan38 joan38 merged commit bfbebc4 into joan38:main Jan 21, 2025
1 check passed
@alexarchambault alexarchambault deleted the non-standard-repositories branch January 21, 2025 00:40
@lihaoyi
Copy link
Contributor

lihaoyi commented Jan 21, 2025

Thanks @alexarchambault @joan38 ! Could you help tag release with this PR?

@joan38
Copy link
Owner

joan38 commented Jan 21, 2025

0.5.0 🚀

lihaoyi added a commit to com-lihaoyi/mill that referenced this pull request Jan 22, 2025
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