-
Notifications
You must be signed in to change notification settings - Fork 185
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
Add rewriting rule RedundantSyntax.finalObject #1496
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR!
It's definitely useful and generic enough to be a built-in check IMHO, but I am not sure this really belongs to DisableSyntax
as something redundant is not a bad smell/feature like the other flags - the rule description reads "Reports an error for disabled features such as var or XML literals.".
Also, since it's redundant, this would really benefit from being a rewrite rule. I believe there are other redundant tokens that could be rewritten. What about starting a RedundantTokens
syntactic rewriting rule to group them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fast turnaround! Could you add some basic docs in /docs/rules/RedundantTokens.md? Thanks!
@@ -0,0 +1,11 @@ | |||
/* | |||
rules = RedundantTokens | |||
RedundantTokens.finalObject = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some negative tests here where the final keyword is used on other symbols would be nice against regressions
scalafix-rules/src/main/scala/scalafix/internal/rule/RedundantTokens.scala
Outdated
Show resolved
Hide resolved
Related: scala/bug#11094 |
Will do! (I'm trying to refrain from opening an editor on a Saturday though so the turnaround won't be as quick...) |
Interesting - does that mean we should deactivate this rule for members of non-final traits/classes on Scala 2.12 and earlier, and/or have that behaviour configurable? |
Since it's on by default and may have unexpected yet limited side effects on the bytecode, I'd say that a reference to that ticket in the docs would be enough. |
Hi @bplommer - will you have some time to follow-up and document this? Thanks! |
Sorry, I forgot all about this! I'll try to finish it off soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's hold the merge until the last moment so docs don't advertise the new rule before it's released.
No description provided.