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

ScalafmtConfig: require version in a config file #2843

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

kitbellew
Copy link
Collaborator

No description provided.

@kitbellew kitbellew changed the title ScalafmtConfig: make version a required field ScalafmtConfig: require version in a config file Nov 2, 2021
@kitbellew kitbellew force-pushed the mver branch 2 times, most recently from 2f907dc to 710ed41 Compare November 2, 2021 17:55
@kitbellew kitbellew requested a review from tgodzik November 2, 2021 18:02
Copy link
Contributor

@tgodzik tgodzik left a 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.
Copy link
Contributor

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?

Copy link
Collaborator Author

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).

Copy link
Contributor

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
extends Error("Version will be required, can't replace with default")
extends Error("Version is required, it can't be replaced with default")

?

Copy link
Collaborator Author

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.

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.

2 participants