Add GitHub shorthand for URL rewrites #128
Conversation
|
Hu, also, I haven't ran scalafmt on the patch yet, so the CI is probably going to complain about that too. |
|
Ok, I've run scalafmt, that part should be ok now |
| @@ -20,4 +20,37 @@ class URLConfiguration | |||
| | | |||
| """.stripMargin | |||
| DiffTest("url", code, "UrlRewrite.stat").foreach(runDiffTest) | |||
|
|
|||
| val githubURL = | |||
| "github:scalacenter/scalafix/1.0" | |||
There was a problem hiding this comment.
Actually, I just realised that we of course just want to unit test this feature. One integration test for the actual http request should be enough. We can create a new tests suite named GitHubUrlRewriteTest inside scalafix-tests/src/test/scala/...
There was a problem hiding this comment.
yep, I realized it while writing it as well. I'll add a unit test and remove this.
| |rewrites = [ | ||
| | "$githubURL" | ||
| |] | ||
| |<<< NOWRAP GitHub shorthand |
There was a problem hiding this comment.
Since we only need to test the String => URL expansion, there's no need to test this.
|
@olafurpg done! I've force-pushed the version of the tests, no more integration tests, just unit tests checking the shorthand expansion. |
| import org.scalatest.{FunSuiteLike, Matchers} | ||
| import metaconfig.Conf | ||
|
|
||
| class GitHubUrlRewriteSuite extends FunSuiteLike with Matchers { |
There was a problem hiding this comment.
I've used FunSuiteLike directly since no scalafix-specific features are needed.
| import org.scalatest.{FunSuiteLike, Matchers} | ||
| import metaconfig.Conf | ||
|
|
||
| class GitHubUrlRewriteSuite extends FunSuiteLike with Matchers { |
|
🎉 |
| val normVersion = version.replaceAll("[^\\d]", "_") | ||
| val fileName = s"${repo.toLowerCase.capitalize}_$normVersion.scala" | ||
| new URL( | ||
| s"https://github.com/$org/$repo/blob/$sha/scalafix-rewrites/src/main/scala/$repo/scalafix/$fileName") |
There was a problem hiding this comment.
I just realized that repo can contain non-alphanumerical characters like - which are unsupported in package names. What do you think @gabro about doing a replaceAll("[^a-zA-Z0-9]", "_")?
There was a problem hiding this comment.
Right, tracking it here: #131
I'll fix it soon.
Closes #125
First working draft. I've setup some integration tests but they fail because the file referenced is not there. Not sure whether these tests make sense, or whether we should just unit test the shorthand expansion. Anyway, the
FileNotFoundexception shows a correct value, so 🎉.@olafurpg take a look and let me know what you think!