-
Notifications
You must be signed in to change notification settings - Fork 278
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: require version
in a config file
#2843
Conversation
version
a required fieldversion
in a config file
2f907dc
to
710ed41
Compare
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.
Looks good, just two quick questions.
* The version of scalafmt to use for this project. Currently not used, the | ||
* plan is to use this field for the IntelliJ+sbt integrations. | ||
* The version of scalafmt to use for this project. Must match the current | ||
* binary version. |
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.
What do you mean by binary version? Scala binary version?
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.
how should i rephrase it? i meant the version of scalafmt that is running (aka, no dynamic resolution at this stage).
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 maybe Must match the current version
?
@@ -48,4 +48,8 @@ object ScalafmtDynamicError { | |||
|
|||
case class UnknownError(cause: Throwable) | |||
extends ScalafmtDynamicError("unknown error", cause) | |||
|
|||
object ConfigVersionRequired | |||
extends Error("Version will be required, can't replace with default") |
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.
extends Error("Version will be required, can't replace with default") | |
extends Error("Version is required, it can't be replaced with default") |
?
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.
there's a nuance here. the configuration file might contain a version so the user could find this error message misleading.
what i'd like to say is, we can't use withRespectVersion
or withDefaultVersion
(whether or not version is present in the configuration file) because they would allow a version to be missing.
No description provided.