Upgrade to scalafmt 3.0.0-RC6 #2315
Conversation
|
The two minor changes have to do with |
|
Note that the other change was to remove the following that are not default: Removing there really helps the readability by aligning the closing paren with the stating symbol. This is especially useful to find the return type like this. |
|
Can we avoid this: @@ -791,3 +845,5 @@
}
- if (uri.scheme == null && scheme != null || uri.scheme != null && scheme == null) {
+ if (
+ uri.scheme == null && scheme != null || uri.scheme != null && scheme == null
+ ) {
return false? Otherwise it seems to look good. |
|
@sjrd Thanks you for the review and positive feedback - I tried the obvious setting but I am not that happy with it so I am checking into the situation. danglingParentheses.ctrlSite = false // default true as seenResult: if (host.indexOf(':') != -1 && host.indexOf(']') == -1 &&
host.indexOf('[') == -1) {
hostVar = "[" + host + "]"
}I am not really happy with this indentation as it is hard to tell the I was hoping for a 4 space continuation or allowing something like the following. + if (host.indexOf(':') != -1 && host.indexOf(']') == -1 &&
+ host.indexOf('[') == -1
+ ) {
hostVar = "[" + host + "]"
} |
|
4 space indentation would be ideal in that situation. |
|
This is now RC4 which allows the desired Update: this is because the wrap would normally indent 2 spaces but because it is indented behind the opening There are 3 places that a long condition plus internal parenthesis cause the anomaly. These could probably be wrapped after the infix and before the open paren java.io.File // line 400
scalanative.regex.Machine // line 301
java.math.BigDecimal // line 1612Edit: I went ahead and fixed these sites and updated the diff so we don't have any unexpected wraps or indents. Now I am convinced that the formatting is ok. 6/11/2021 (Now diff is 97853) New diff: https://gist.github.com/ekrich/2c73e695f1e6487ff183e4bd2ca4231e We have gone from 99053 lines in the diff to 97883. @sjrd This is ready for a second look. If ok I can run the update and fix a few comment sites where they got wrapped after the closing |
|
Final diff. Just need to format and then check in to complete the PR. https://gist.github.com/ekrich/b6e1727f2fabadfd0f155377891c603b |
0e8f2fe to
e1eaabf
Compare
There was a problem hiding this comment.
Hey, thank you for your work done in this area. Can you please run scalafmt with new settings and commit changes here, it would be easier to check actually difference and comment on them (also needed for check-lint CI job)
Also, based on your latest diff, can we stick to the previous alignment of rhs for block or variables/methods?
- def cloneArray(array: Array[scala.Long]) = array.clone
- def cloneArray(array: Array[scala.Float]) = array.clone
- def cloneArray(array: Array[scala.Double]) = array.clone
...
+ def cloneArray(array: Array[scala.Long]) = array.clone
+ def cloneArray(array: Array[scala.Float]) = array.clone
+ def cloneArray(array: Array[scala.Double]) = array.clone
Otherwise looks good to me
| @@ -17,17 +17,20 @@ object in { | |||
| type sockaddr_in = CStruct4[socket.sa_family_t, // sin_family | |||
| in_port_t, // sin_port | |||
| in_addr, // sin_addr | |||
| CArray[Byte, _8]] // sin_zero, Posix allowed | |||
| CArray[Byte, _8] // sin_zero, Posix allowed | |||
| ] | |||
There was a problem hiding this comment.
Can we try to revert that, it looks a bit weird I think. I think in Scala.js ending square bracket sticks to the right side of the last type.
There was a problem hiding this comment.
These will all be indented left in a more Scala 3 style so it won't look like it is hanging out there.
| CStruct2[sa_family_t, // sa_family | ||
| CArray[CChar, _14]] // sa_data, size = 14 in OS X and Linux | ||
| type sockaddr = CStruct2[sa_family_t, // sa_family | ||
| CArray[CChar, _14] // sa_data, size = 14 in OS X and Linux |
There was a problem hiding this comment.
Can we somehow force the alignment of these parameter types?
There was a problem hiding this comment.
Same here. I will go ahead and format now so you can see the whole change. I wrapped a few things and made changes in the PR prior to mass formatting to make sure everything will look good.
Of course, it will be quite a lot of change as explained in this PR but I think it really actually makes readability better.
There was a problem hiding this comment.
The example above looks like this now.
type sockaddr = CStruct2[
sa_family_t, // sa_family
CArray[CChar, _14] // sa_data, size = 14 in OS X and Linux
]Having the close ] and or ) in other place lined up is a key to better readability. I wasn't a believer until I saw it in action.
I am not sure if you are talking about the = getting lined up but that is actually a big contributor to whitespace diffs. I think in some cases it increases readability but in others it can be pretty ugly. I was also wanting to more closely match the Scala.js style. Not sure if that is what you were asking.
There was a problem hiding this comment.
Looks nice!
Yes, that exactly what I was asking about, alignment of assignments using =. I personally prefer them aligned. As you pointed out in the first comment change of this alignment is responsible for most of the diff changes (~23k LOC changed out of ~61k total). However, I've also observed that in Scala.js they're not aligned. If we want to create a common code style across both projects we can follow that convention.
@sjrd what do you about that?
There was a problem hiding this comment.
Thanks. I thought I liked that too but wanted to go with the Scala.js = non alignment and there are some benefits too like I mentioned, reduced diffs etc. I also wanted to try and go with less configuration. I spent a ton of time trying to upgrade scalafmt and never got the result that anyone could buy. Spending a ton of time trying to upgrade or tweak the config to get the perfect format is a losing battle and kind of goes against the philosophy of a formatter. You still sometimes have to preformat or change things a bit to get scalafmt to do what you want. I wish that the default was good enough but we are down to very few extra options which is good.
Yes, I think that Scala.js is not forcing alignment of the closing ] or ) on a new line but when you look the direction that Scala 3 is going with optional braces and such, I think this is the direction we should go. Because the indents after are always readable, I think this is a good compromise and I think it is actually more readable. Things that fit in 80 chars stay the same, just wrapping is different. Looking at the Scala.js style guide we would just have the closing ) always on a new line which is different. For example, the reportError would have the ) on a new line. https://github.com/scala-js/scala-js/blob/master/CODINGSTYLE.md#long-expressions-with-binary-operators
Some of the more subtle rules in Scala.js won't be possible for scalafmt and certainly using scalafmt has some advantages and disadvantages as well. I also think we tend to get used to things over time. I basically had two goals. 1) make the format very similar to the Scala.js guide. 2) Have the smallest configuration file as possible. I also didn't want the diff to be completely out of control. It is big, but I think in a good way.
|
I realize this was already approved but there was an outstanding fix needed that caused Fixed by scalameta/scalameta#2398 which worked perfectly. |
9ab6125 to
7bfc92d
Compare
|
Eventhough this is approved and merged, we have received some more really good information from @sjrd in this PR: scala-js/scala-js#4522 |
* Add code block so ascii art is maintained * Reworked with newest version * Update to 3.0.0-RC4 and update formats * Pre format wrapping if locations * Remove extra * in comment * Add blank line after @example to keep formatting * Fix if site wrapping to play nice with scalafmt * Pre wrap type definition with trailing comment that describes the last type element * Move comment above type for formatting * Format using new style - rebase * Touch up return type wrapping * Update to 3.0.0-RC6 for fixes related to scaladoc errors * Format JUnit upgrade change
I think this is a very good proposal / PR for an upgrade. Rather than try and minimize the diff we rather adopt a format much like the style guide in Scala.js - https://github.com/scala-js/scala-js/blob/master/CODINGSTYLE.md
The other thing that is really great about this format is that the configuration file is virtually default and aligns quite nicely with future Scala 3 code style.
The one thing that differs most is that
scalafmtcan do binpacked or not and this does not use binpacked. If the line is too long it wraps all the arguments whereas Scala.js allows either format. Scalafmt now allows inline formats so in generated code for 22 args for example they could be specially binpacked if desired.Before anyone panics because of the big diff, here are the major causes of the diff - all are positive except one which can be fixed after the fact which will be explained later in this analysis.
Features that cause changes. There are 192,604 lines of Scala code without the Scala lib which is 163,604 LOC. The diff itself is 99,053 lines long.
This means that
99,053 - 61674 = 37,379 / 2 = 18,670LOC left. These look to be almost exclusively based on the change to remove the following:This change avoids alignment changes when a symbol is changed as the params list is indented in a fixed fashion. The largest chunk of changes is aligning equals which contributes to whitespace changes anytime longer variable names are changed. Case
=>are still aligned but this is a default setting and is the same as in Scala.js as well.These changes not only align to Scala.js but also to
scalafmtstandard format and align nicely to Scala 3 optional braces.I hope this make it easier for reviewers to accept this large change.
Diff: https://gist.github.com/ekrich/8cf45c1523b018a60acdd27f60d609a0