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

RedundantSyntax removes unnecessary string interpolator #1602

Merged
merged 2 commits into from Jun 9, 2022

Conversation

gontard
Copy link
Contributor

@gontard gontard commented Apr 17, 2022

Idea from gitter:

- println(s"Hi!")
+ println("Hi!")

@gontard gontard force-pushed the unnecessary_string_interpolator branch from 59ef8ea to c805905 Compare April 17, 2022 21:24
@gontard gontard changed the title Rule removing unnecessary string interpolator RedundantSyntax removes unnecessary string interpolator Apr 17, 2022
@gontard gontard force-pushed the unnecessary_string_interpolator branch 2 times, most recently from acfed20 to cc35111 Compare April 17, 2022 21:35
@gontard gontard marked this pull request as ready for review April 17, 2022 21:50
"""foo
|bar"""
b = s"""foo $a bar"""
b = s"""$a"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a is a String

Suggested change
b = s"""$a"""
b = a

but i guess it's not doable with a syntactic rule

Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

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

👋 thanks for putting this together!

Interestingly, like any generic syntactic rule, this could live in scalafmt, but it seems that it was never requested...

A good use-case for this rule would be to fail (and not rewrite) after writing by accident println(s"variable: {variable})". Unfortunately, it's not possible to configure scalafixOnCompile to --check rather than rewrite (early feedback would be the best in that case). #1375 could be extended to support this I guess, so if the need comes we can do it at the framework level rather than the rule one (adding an adhoc lint boolean would be cumbersome) anyway.

docs/rules/RedundantSyntax.md Outdated Show resolved Hide resolved
finalObject: Boolean = true
finalObject: Boolean = true,
@Description("Remove unnecessary string interpolator")
stringInterpolator: Boolean = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 I was wondering if we could make this more extensible:

Suggested change
stringInterpolator: Boolean = true
stringInterpolators: Seq[String] = Seq("s", "f", "raw")

But considering raw has a special case and all the custom interpolators I have used wouldn't be eligible for such rewrites, it's probably useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that too... but i came to the same conclusion

@gontard gontard force-pushed the unnecessary_string_interpolator branch from cc35111 to b2d7256 Compare April 19, 2022 07:28
@gontard gontard requested a review from bjaglin April 19, 2022 12:05
Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Not merging right away to avoid updating the docs too much ahead of next release

@bjaglin bjaglin merged commit 074e614 into scalacenter:main Jun 9, 2022
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.

None yet

2 participants