-
Notifications
You must be signed in to change notification settings - Fork 277
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
ScalafmtConfig: allow fileOverride
shortcuts, set cross-build dialects
#2902
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.
Just a minor comments, but otherwise looks super useful!
docs/configuration.md
Outdated
|
||
If set, for minor versions, dialect will be determined automatically, as if with | ||
`fileOverride { "lang:scala-2.13" = scala213 }`. Currently, supports | ||
scala versions 2.10 through 2.13. |
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.
Why not also Scala 3? The convention would be src/main/scala-3
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.
scala-3 is a major version. we can't deterministically figure out which specific minor dialect to use, hence automatic dialect is only for minor versions.
are there scala 3.0x already in active use?
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.
We started using that convention here: https://github.com/scalameta/metals/tree/main/mtags/src/main/scala-3/scala/meta/internal
It should be less common for users to use scala-3.0
or scala-3.1
, but I suppose it could happen. However I don't expect the convention to be different that in Scala 2.
We could default for now to dialect Scala3
for now I think. We will need to update it anyway when some drastic things happen, though 3.1
didn't change anything syntactic still.
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.
do you know how sbt cross build determines source directory to use for a given scala version by default? that's the only thing we should realistically support here.
for metals, i found this complex, non-default logic: https://github.com/scalameta/metals/blob/main/build.sbt
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.
specifically, I'm looking for scala3 version-specific subdirs like https://www.scala-sbt.org/1.x/docs/Cross-Build.html#Scala-version+specific+source+directory
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.
you mean, not just with mtags but any default cross build?
also, if it works, the difference is likely this: sbt knows the dialect it's building, and just needs to find the source directories for it. however, scalafmt is opposite: knows the directory but doesn't know the dialect.
it's easy to determine if from scala-2.13; what should it be for scala-3?
and for this logic (https://www.scala-sbt.org/1.x/docs/Cross-Build.html#Scala-version+specific+source+directory), how will they apply it to scala 3? would it expect scala-3.10 for 3.1.0, or scala-3.1 (since the migration guide says scala3 uses major.minor.patch instead of epoch.major.minir for scala2).
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.
ok, found this: sbt/sbt#6424
above the changes, lines 745 and 760, they are reading sources from scalaBinaryVersion
. i don't know if default scalaBinaryVersion
for dotty is scala-3
or scala-3.1
or scala-3.1.0
.
i think right now we can do one of two things:
- add dialect support for
scala-3.0
andscala-3.1
as that would allow us to determine the dialect reasonably - revisit this case later, when we have a better understanding.
what would you prefer, @tgodzik?
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.
I checked the default cross build sbt behaviour it recognizes scala-3
directory by default in a sample project. It doesn't work for scala-3.0
etc.
The behaviour in sbt/sbt#6424 was for release candidates that no longer applies.
I think it's safe to assume that scala-3
will always work. Having 3.0
and 3.1
directories is no in the plans AFAIK
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.
ok, done. added scala-3
.
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!
Also, introduce additional shortcuts for `fileOverride`.
Fixes #2882.