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

adding possibility to specify custom repositories for coursier (#1521) #1586

Merged
merged 1 commit into from
Dec 6, 2019

Conversation

slivkamiro
Copy link
Contributor

I'm kind of lost in tests. I would need help with writing a test for ScalafmtDynamicDownloader, if necessary (I haven't found any).
I'm also struggling with sbt-scalafmt part. I can't find a way how to map resolvers configuration into list of string urls. Any ideas?

@@ -91,7 +91,7 @@ class ScalafmtDynamicDownloader(
"https://oss.sonatype.org/content/repositories/snapshots"
),
MavenRepository.of("https://oss.sonatype.org/content/repositories/public")
)
) ++ customRepositories

Choose a reason for hiding this comment

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

i think custom repos should either override the defaults, or have higher priority - one usecase of this (ours) is that oss repositories are disabled and must go through our own artifactory and it causes timeouts when trying to download stuff from OSS ... therefore it makes sense to try custom repositories first and fallback on defaults

Copy link
Contributor Author

@slivkamiro slivkamiro Dec 6, 2019

Choose a reason for hiding this comment

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

timeouts are good point, will move it in the front then

@slivkamiro slivkamiro force-pushed the resolvers branch 2 times, most recently from a724e32 to 2c9c15f Compare December 6, 2019 08:43
Copy link
Collaborator

@poslegm poslegm left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

LGTM

@olafurpg @tanishiking Do you see any pitfalls?

@sideeffffect
Copy link

@slivkamiro thank you so much for this!
I know this is just scalafmt proper, but are you perhaps considering implementing support for this in sbt-scalafmt plugin? It would be great, if the -Dsbt.repository.config=/.../repositories -Dsbt.override.build.repos=true overrides were respected.

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! One comment, otherwise looks great!

/**
* Use this repositories to resolve dependencies.
*/
Scalafmt withRepositories(String ... repositories);
Copy link
Member

Choose a reason for hiding this comment

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

Let's make it clear that these must be Maven repositories

Suggested change
Scalafmt withRepositories(String ... repositories);
Scalafmt withMavenRepositories(String ... repositories);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem, I will amend the commit if you don't mind

@slivkamiro
Copy link
Contributor Author

@slivkamiro thank you so much for this!
I know this is just scalafmt proper, but are you perhaps considering implementing support for this in sbt-scalafmt plugin? It would be great, if the -Dsbt.repository.config=/.../repositories -Dsbt.override.build.repos=true overrides were respected.

Yes, see scalameta/sbt-scalafmt#73

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you for this contribution!

@poslegm poslegm merged commit 5f8ac94 into scalameta:master Dec 6, 2019
@slivkamiro slivkamiro deleted the resolvers branch December 9, 2019 08:44
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.

5 participants