Deprecate v0.Rule and remove it from tests

[mlachkar]

This commit includes:

• Deprecating v0.Rule
• Deleting both SemanticRuleSuite and SyntacticRuleSuite
• Breaking changes in AbstractSyntacticRuleSuite: We consider it's acceptable, we haven't seen any usage of it on github projects, and its usage is not even documented.

Next step: Create a migration rule to help to migrate from SemanticRuleSuite to AbstractSemanticRuleSuite.

[bjaglin]

I am surprised to see scalafix.v0.Rule was not deprecated, we should definitely do it 👍

Regarding the removal of SyntacticRuleSuite and the significant signature changes in AbstractSyntacticRuleSuite, I am not sure there is a need to do that without a deprecation heads up first (so that any corporate user would report back here if it's a big problem)?

The testkit is very little documented (apart from the tutorial in the docs that uses https://github.com/olafurpg/named-literal-arguments which should probably be moved to the scalacenter org) so I wonder if it wouldn't be a good occasion to move away from Scalatest (both for syntactic and semantic rules) as discussed in #1172 (comment) (causing scala/community-build#1290).

We could deprecate all Scalatest helpers to advertize new munit-based ones in a v1 package matching the rules (where we could also but the semantic helper, even though it is not coupled to the v1 model?). And for Scalafix 1.0, we drop the Scalatest helpers/dependency?

[bjaglin]

obsolete doc

[mlachkar]

Even in the tutorial about tests, and the repo namedLiteral, they only use this

package fix

import scalafix.testkit.SemanticRuleSuite

class RuleSuite extends SemanticRuleSuite() {
  runAllTests()
}

I do like the simplicity of the tests in scalafix!

• we can first deprecate AbstractSyntacticRuleSuite (even if I think it's not used, since it's not documented, and scalafix.g8 only uses SemanticRuleSuite or AbstractSemanticRuleSuite).

You said we should deprecate all Scalatest helpers to advertize new munit-based , so If we consider that most people use AbstractSemanticRuleSuite, we only need to advertise a new runAllTests() ?

[bjaglin]

Even in the tutorial about tests, and the repo namedLiteral, they only use this

Unfortunately, SemanticRuleSuite works only with old versions of ScalaTest, see https://github.com/scalacenter/scalafix/pull/1176/files

even if I think it's not used, since it's not documented, and scalafix.g8 only uses SemanticRuleSuite or AbstractSemanticRuleSuite

You're right, it's acceptable to drop it since it only impacts rule authors - what matters is to be more conservative with v0.Rule users, which you did by deprecating.

You said we should deprecate all Scalatest helpers to advertize new munit-based , so If we consider that most people use AbstractSemanticRuleSuite, we only need to advertise a new runAllTests() ?

Yes, advertize new traits with a runAllTests that does not use ScalaTest but does exactly the same thing. I am not familiar with munit so there might be a smarter way to provide the test runners to the client.

Actually, regarding the AbstractSyntacticRuleSuite breaking changes / rewrite against scalafix.v1.Rule, I think it would make sense to drop the distinction syntactic/semantic rules for testing and keep only one RuleSuite test runner. The only reason I can think for keeping a syntactic-only helper is the UX: ignoring sbt-scalafix's ScalafixTestkitPlugin which makes the usage of AbstractSemanticRuleSuite transparent/easy by injecting the proper properties, would the current AbstractSemanticRuleSuite still work to test syntactic rule if there is no prior compilation of test inputs and injection of properties?

[mlachkar]

SemanticRuleSuite is deprecated. we can maybe start by removing just SemanticRuleSuite and SyntacticRuleSuite, this way we don't depend on old versions of scalatest?

[bjaglin]

SemanticRuleSuite is deprecated. we can maybe start by removing just SemanticRuleSuite and SyntacticRuleSuite, this way we don't depend on old versions of scalatest?

By bumping ScalaTest here, we make it very hard for ScalaTest 3.0 clients to use the testkit (I think it would still be possible to use AbstractSemanticRuleSuite against 3.0 on the client-side but they would have to force the downgrade to prevent eviction). I don't know to which extend Twitter still relies on Scalafix and whether they have upgraded since #1172. But I think it would be a good time to move on indeed.

[mlachkar]

the impact of removing SemanticRule would be as easy to fix as this for authors rule
spotify/scio#3796

[mlachkar]

This commit includes:

Deprecating v0.Rule
Deleting both SemanticRuleSuite and SyntacticRuleSuite
Breaking changes in AbstractSyntacticRuleSuite: We consider it's acceptable, we haven't seen any usage of it on github projects, and its usage is not even documented.
Next step: Create a migration rule to help to migrate from SemanticRuleSuite to AbstractSemanticRuleSuite.