Allow package when referencing rules in github #1817
Conversation
| import metaconfig.Conf | ||
| import metaconfig.ConfError | ||
| import metaconfig.Configured | ||
| import metaconfig.Configured.Ok | ||
|
|
||
| object GitHubUrlRule { | ||
|
|
||
| private val DefaultBranch = "master" |
There was a problem hiding this comment.
Should this actually be changed to main ?
There was a problem hiding this comment.
That sounds like a good idea to favor main over master. However, for backward compatibility with all repos and instructions out there, I would like to keep the fallback to master if we get a 404 on main. Maybe for another PR?
There was a problem hiding this comment.
Thanks for the PR @RustedBones, and sorry for the delay. I am definitely in favor of merging this after a few minor adjustments 👍
| import metaconfig.Conf | ||
| import metaconfig.ConfError | ||
| import metaconfig.Configured | ||
| import metaconfig.Configured.Ok | ||
|
|
||
| object GitHubUrlRule { | ||
|
|
||
| private val DefaultBranch = "master" |
There was a problem hiding this comment.
That sounds like a good idea to favor main over master. However, for backward compatibility with all repos and instructions out there, I would like to keep the fallback to master if we get a 404 on main. Maybe for another PR?
| @@ -37,6 +37,11 @@ class GitHubUrlRuleSuite extends AnyFunSuite with DiffAssertions { | |||
| "https://raw.githubusercontent.com/someorg/some-repo/master/scalafix/rules/" + | |||
| "src/main/scala/fix/RuleName.scala" | |||
| ) | |||
| check( | |||
| "github:someorg/some-repo/version.RuleName", | |||
There was a problem hiding this comment.
could we add a test with several segments? like my.package.RuleName
There was a problem hiding this comment.
also, a test with the sha maybe? (it's not documented but I would expect the most verbose/complicated syntax to support all features)
| @@ -32,17 +36,21 @@ object GitHubUrlRule { | |||
| private def guessGitHubURL( | |||
| org: String, | |||
| repo: String, | |||
| filename: String, | |||
| rule: String, | |||
There was a problem hiding this comment.
maybe className (here and above) to highlight that . is used as separator and that extension is not provided?
| rule: String, | |
| className: String, |
| @@ -54,35 +62,27 @@ object GitHubUrlRule { | |||
| private val GitHubFallback = | |||
| """github:(.*)""".r | |||
|
|
|||
| private val alphanumerical = "[^a-zA-Z0-9]" | |||
| private val NonAlphaNumeric = "[^a-zA-Z0-9]" | |||
docs/developers/tutorial.md
Outdated
| @@ -654,6 +654,7 @@ The expansion rules for `github:org/repo` are the following: | |||
| | `github:org/repo` | `scalafix/rules/src/main/scala/fix/Repo.scala` | | |||
| | `github:org/some-repo` | `scalafix/rules/src/main/scala/fix/SomeRepo.scala` | | |||
| | `github:org/repo/RuleName` | `scalafix/rules/src/main/scala/fix/RuleName.scala` | | |||
| | `github:org/repo/package.RuleName` | `scalafix/rules/src/main/scala/fix/package/RuleName.scala` | | |||
There was a problem hiding this comment.
if package is explicit, shouldn't we remove the fix prefix?
| | `github:org/repo/package.RuleName` | `scalafix/rules/src/main/scala/fix/package/RuleName.scala` | | |
| | `github:org/repo/package.RuleName` | `scalafix/rules/src/main/scala/package/RuleName.scala` | |
There was a problem hiding this comment.
Looks that scalafix requires the root package to be fix.
When only the rule name is provided, it searches in the fix package, this changes keep the same behavior
There was a problem hiding this comment.
Looks that scalafix requires the root package to be fix.
I am not aware of this constraint in scalafix-core, and none of the built-in rules is rooted in fix. Can you elaborate where you observed this limitation?
When only the rule name is provided, it searches in the fix package, this changes keep the same behavior
I agree that the existing behavior should remain, but it feels strange that the user provides a package that is only a suffix. If the example would look like this, would it make more sense?
| | `github:org/repo/package.RuleName` | `scalafix/rules/src/main/scala/fix/package/RuleName.scala` | | |
| | `github:org/repo/fix.package.RuleName` | `scalafix/rules/src/main/scala/fix/package/RuleName.scala` | |
There was a problem hiding this comment.
My bad, I double checked scalafix uses the rule name only, regardless of the class name/package.
I agree with the suggestion. Will update the PR
| | `github:org/some-repo` | (on `master`) `scalafix/rules/src/main/scala/fix/SomeRepo.scala` | | ||
| | `github:org/repo/RuleName` | (on `master`) `scalafix/rules/src/main/scala/fix/RuleName.scala` | | ||
| | `github:org/repo/com.example.RuleName` | (on `master`) `scalafix/rules/src/main/scala/com/example/RuleName.scala` | | ||
| | `github:org/repo/RuleName?sha=main` | (on `main`) `scalafix/rules/src/main/scala/fix/RuleName.scala` | |
Referencing scalafix rules does not currently allow rules to be located in a package other than
fix.Eg. in scio we've grouped rules in packages corresponding to the breaking version.
This change allows to reference a rule based on its package name (stripping the required
fixpackage prefix)