-
Notifications
You must be signed in to change notification settings - Fork 607
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
Upgrade Scalafmt to 3.8.3 #4566
base: main
Are you sure you want to change the base?
Conversation
n: Int | ||
)(gen: (Int) => T | ||
)( | ||
n: Int |
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 prettier
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.
the spot checks of what i saw look nice. The alignment on =>
is definitely more aggressive, that would be the only thing I'd wonder if you want to turn off but I also like it if we're erring on the side of alignment
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 definitely like putting the case
on the same line! Overall I think its good.
def known: Boolean = false | ||
def get: Int = None.get | ||
def known: Boolean = false | ||
def get: Int = None.get |
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.
It's a bit strange to me to align the types, but not the =
? But I suppose that might be a much larger diff, which is fine to avoid.
37c9adc
to
5f7e78b
Compare
As you can see, there's a bit of a diff. I've done the best I can to minimize it but there's some diff.
It seems to try to keep argument lists on one line, which I like, but it does it even if an early argument list was split between multiple lines which is a bit weird.
It also likes keeping
case
on the same line as the{
which I like, although occasionally that leads to longish lines. Anyway, we can try to massage this further or just go with it. I think it's fine but am curious what others think.We will need to bump Scalafmt soon in order to support Scala 3.
Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x
,5.x
, or6.x
depending on impact, API modification or big change:7.0
)?Enable auto-merge (squash)
, clean up the commit message, and label withPlease Merge
.Create a merge commit
.