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

Run Scala 3 migrate on mtags sources #2617

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Mar 16, 2021

While testing out https://github.com/scalacenter/scala3-migrate I decided to run it on mtags module to see if there is anything that we can improve.

This included:

  • changes to scalac options for Scala 3
  • scalafix rewrites that deals with 2.13 deprecations, which weren't sctrictly neccessary, but might be usful for the future if we decide to move some of the Scala 2 code
  • I also added a test case for one of Scala 3 issues we had (hovers on extension methods)

@tgodzik tgodzik requested review from ckipp01 and dos65 March 16, 2021 15:49
While testing out https://github.com/scalacenter/scala3-migrate I decided to run it on mtags module to see if there is anything that we can improve.

This included:
- changes to scalac options for Scala 3
- scalafix rewrites that deals with 2.13 deprecations, which weren't sctrictly neccessary, but might be usful for the future if we decide to move some of the Scala 2 code
- I also added a test case for one of Scala 3 issues we had (hovers on extension methods)
@@ -71,6 +71,21 @@ class HoverScala3TypeSuite extends BaseHoverSuite {
|""".stripMargin.hover
)

// https://github.com/scalameta/metals/issues/1991
check(
"extension-methods",
Copy link
Member

Choose a reason for hiding this comment

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

This one was fixed in dotty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be fixed in 3.0.0-RC2, so I wanted to add the test case to close the issue when updating.

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Just a quick question, but other than that LGTM!

@@ -62,7 +62,7 @@ object Identifier {
def backtickWrap(s: String): String = {
if (s.isEmpty) "``"
else if (s(0) == '`' && s.last == '`') s
else if (needsBacktick(s)) '`' + s + '`'
else if (needsBacktick(s)) "" + ('`') + s + '`'
Copy link
Member

Choose a reason for hiding this comment

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

I've seen this in a couple places where it prefaces with an empty string. What's the reasoning for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

huh, alright, thanks for linking that. I hope this doesn't become confusing for someone in the future that comes across this and wonders why there is a "" prefacing it. I can see myself seeing that just removing it 😆

Copy link
Member

Choose a reason for hiding this comment

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

Hm. I've just realized that this line might look less weird if at least the first backtick was a type of String:

else if (needsBacktick(s)) "`"+ s + "`"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ach yes, I didn't notice the comments when merging, sorry!

@tgodzik tgodzik merged commit 4748fcd into scalameta:main Mar 17, 2021
@tgodzik tgodzik deleted the migrate branch March 17, 2021 10:33
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.

3 participants