-
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
Use a lower case for runner dialects in the error message #2881
Use a lower case for runner dialects in the error message #2881
Conversation
@@ -97,7 +97,7 @@ object ScalafmtRunner { | |||
known.find(_.value eq dialect).map(_.source) | |||
|
|||
def getUnknownError = { | |||
val knownStr = known.map(_.source).mkString(",") | |||
val knownStr = known.map(_.source.toLowerCase).mkString(",") |
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.
this is the only change we should keep. why do we need the others?
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.
Are you about the ordering of runner dialects?
It's quite natural to have an order of runner dialects for the Scala first and then for SBT, I suppose. Just as in docs.
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.
Or if you are convinced that it's mindless, I doubtless will fix it :)
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 are worried about the presentation of dialects in some error message. therefore, take care of that and only that, don't change the internal list of dialects.
the reason is that next time someone adds a dialect, he will mess it up for you.
so you can lowercase it here, you can apply sorting etc. but don't change known
.
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, now it's only lowercase here.
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're correct. but it doesn't matter, it will help people do a mental binary search rather than a linear scan.
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.
Hmm, if we have the Seq[WithName]
then calling the withName
is redundant? Am I missing some things? Anyway, compilation succeeded without it.
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.
perhaps i misunderstood what you want to do in the first place. withName
was taking a lowercase version of the name. did you NOT care about the lowercase name?
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.
Wow, shame on me. I'm half asleep. You're 100% right!
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.
Hope, now all is ready to merge these two lines 😄
8486175
to
033305a
Compare
033305a
to
b250dd3
Compare
Just to synch it with docs https://scalameta.org/scalafmt/docs/configuration.html#scala-dialects.