Replace java reflection with a macro-based solution #4280
Conversation
jchyb
force-pushed
the
scala-native/dialect-macro
branch
3 times, most recently
from
2f9cb5a
to
25fe84e
Compare
There was a problem hiding this comment.
one request: if you are actively working on this code, could you please submit it as draft during that time, before marking it ready for review when you are done?
my review comments keep getting rejected because the patch had been updated in the meantime, it's a bit counterproductive.
scalafmt-macros/shared/src/main/scala/org/scalafmt/config/DialectMacro.scala
Outdated
Show resolved
Hide resolved
scalafmt-macros/shared/src/main/scala/org/scalafmt/config/DialectMacro.scala
Outdated
Show resolved
Hide resolved
scalafmt-macros/shared/src/main/scala/org/scalafmt/config/DialectMacro.scala
Outdated
Show resolved
Hide resolved
scalafmt-macros/shared/src/main/scala/org/scalafmt/config/DialectMacro.scala
Outdated
Show resolved
Hide resolved
scalafmt-macros/shared/src/main/scala/org/scalafmt/config/DialectMacro.scala
Outdated
Show resolved
Hide resolved
scalafmt-core/shared/src/main/scala/org/scalafmt/config/ScalafmtRunner.scala
Outdated
Show resolved
Hide resolved
|
Apologies for the previous force pushes - I submitted as a non-draft because I was confident in this, but it ended up breaking the CI in places I did not predict while PRing, so I tried to fix this |
jchyb
force-pushed
the
scala-native/dialect-macro
branch
from
25fe84e
to
0036be4
Compare
There was a problem hiding this comment.
thank you, this looks really good. i had one small question about the name, but another is: is it possible to add some unit tests for this? perhaps not an exhaustive list of all withXxx methods, as they might change with time, but at least test for a subset...
scalafmt-macros/shared/src/main/scala/org/scalafmt/config/DialectMacro.scala
Outdated
Show resolved
Hide resolved
|
About adding the tests - sure! Would it be better to add more general ones in |
i think all tests are in scalafmt-test. if that's true, then i'd follow the same. if that's not true, then up to you. |
jchyb
force-pushed
the
scala-native/dialect-macro
branch
from
0036be4
to
5570486
Compare
scalafmt-tests/src/test/scala/org/scalafmt/config/ConfigDialectOverrideTest.scala
Show resolved
Hide resolved
scalafmt-tests/src/test/scala/org/scalafmt/config/ConfigDialectOverrideTest.scala
Show resolved
Hide resolved
scalafmt-macros/shared/src/main/scala/org/scalafmt/config/DialectMacro.scala
Outdated
Show resolved
Hide resolved
scalafmt-macros/shared/src/main/scala/org/scalafmt/config/DialectMacro.scala
Outdated
Show resolved
Hide resolved
scalafmt-tests/src/test/scala/org/scalafmt/config/ConfigDialectOverrideTest.scala
Show resolved
Hide resolved
scalafmt-tests/src/test/scala/org/scalafmt/config/ConfigDialectOverrideTest.scala
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,34 @@ | |||
| package org.scalafmt.config | |||
There was a problem hiding this comment.
@tgodzik non-blocking comment for this pr: should we consider copying this functionality into scalameta? i know you have been working on making that working with scala3, perhaps there's a way to have two macro-based implementations, one for scala2 and another for scala3.
scalafmt-macros/shared/src/main/scala/org/scalafmt/config/DialectMacro.scala
Outdated
Show resolved
Hide resolved
jchyb
force-pushed
the
scala-native/dialect-macro
branch
2 times, most recently
from
948a4bf
to
9bf1d65
Compare
scalafmt-tests/src/test/scala/org/scalafmt/config/ConfigDialectOverrideTest.scala
Show resolved
Hide resolved
scalafmt-tests/src/test/scala/org/scalafmt/config/ConfigDialectOverrideTest.scala
Show resolved
Hide resolved
scalafmt-tests/src/test/scala/org/scalafmt/config/ConfigDialectOverrideTest.scala
Outdated
Show resolved
Hide resolved
jchyb
force-pushed
the
scala-native/dialect-macro
branch
from
9bf1d65
to
de9db3b
Compare
scalafmt-tests/src/test/scala/org/scalafmt/config/ConfigDialectOverrideTest.scala
Show resolved
Hide resolved
scalafmt-core/shared/src/main/scala/org/scalafmt/config/ScalafmtRunner.scala
Show resolved
Hide resolved
Necessary for cross compilation with scala native, since it does not offer any reflection functionalities. Instead of the previous method, we create a mapping between strings (pointed out by the dialectOverride in scalafmt.conf) and methods that allow us to replace dialect values.
jchyb
force-pushed
the
scala-native/dialect-macro
branch
from
de9db3b
to
2021273
Compare
|
thank you for your patience! |
Necessary for cross compilation with Scala Native, since it does not offer any reflection functionalities.
Instead of the previous method, we create a mapping between strings (pointed out by the dialectOverride in scalafmt.conf) and methods that allow us to replace dialect values.
With that we still have the previous dialectOverride functionality that we do not have to manually maintain/expose and we avoid java reflection (which should make things faster even on the JVM).
Part of the Scala Native cross compilation effort (#4279)