Switch to swift-format #1655
Switch to swift-format #1655
Conversation
|
WDYT @FranzBusch? |
| @@ -0,0 +1,58 @@ | |||
| { | |||
There was a problem hiding this comment.
I think this file is slightly different than the one in swift-certificates. Can we align on one please.
There was a problem hiding this comment.
Aligning on formatting rules will be a significant amount of churn (52,000 lines) that I don't think is worth doing. The aim of this PR is to reduce churn by switching to a tool which produces more stable formatting; in that light the rules chosen follow the existing ones where possible to minimise the changes.
There was a problem hiding this comment.
I see. On the other hand we are planning to do some significant work in gRPC and that might be a good opportunity to do this one time churn. Having an aligned formatting file across our repos is IMO worth it in the long run. We could take this churn and just tell git to ignore this one commit in blames so it shouldn't even show up.
There was a problem hiding this comment.
I'm not convinced: we still need to work on and maintain v1 for quite some time so will still bear the cost of the significant churn. Formatters let users avoid having to do the manual work of sticking to style guidelines so the benefit of having a consistent style across projects is very low, IMO.
There was a problem hiding this comment.
Sure we have to maintain the v1 but this is a one time churn right? If we decide that swift-format is the future for our project then IMO we should take that one time churn once and then have everything aligned.
There was a problem hiding this comment.
Sure, it's one time but it's a very large change. As I said above, I think the benefit of having consistent style across projects is low, especially as we have tools which enforce the style for you.
What we're effectively debating is which style to use. I would much prefer we stick to the style used in this PR because it minimises the changes made, so much so that they can be reviewed with relative ease.
| @@ -16,43 +16,98 @@ | |||
|
|
|||
| set -eu | |||
|
|
|||
| function log() { printf -- "** %s\n" "$*" >&2; } | |||
There was a problem hiding this comment.
Same here. Can we align the script with the one in swift-certificates?
There was a problem hiding this comment.
This divergence is useful: this version checks out, builds, and caches an appropriate version of swift-format making it easier for new developers to run the formatter (they don't need to know what version to get, how to check it out, etc.).
There was a problem hiding this comment.
I agree that this here does more. We should bring it over to the other repos IMO. I would love to have an aligned approach across all of our repos.
There was a problem hiding this comment.
Yeah, @dnadoba and I talked about this a little while ago – I believe he was interested in this scripts ability to checkout/build/cache the formatter.
| @@ -180,7 +181,7 @@ func makeClient( | |||
| .withTLS(trustRoots: .certificates([caCert.certificate])) | |||
| #else | |||
| fatalError("'useTLS: true' passed to \(#function) but NIOSSL is not available") | |||
| #endif // canImport(NIOSSL) | |||
| #endif // canImport(NIOSSL) | |||
There was a problem hiding this comment.
This looks like a bug in swift-format. There should only be one space.
There was a problem hiding this comment.
I'm not sure it is; it's quite common to include two spaces before an inline comment.
| @@ -415,7 +419,7 @@ internal final class AsyncServerHandler< | |||
| internal func receiveInterceptedMetadata(_ headers: HPACKHeaders) { | |||
| switch self.interceptorStateMachine.interceptedRequestMetadata() { | |||
| case .forward: | |||
| () // continue | |||
| () // continue | |||
| self.format.windowBits, // window size, i.e. deflate/gzip | ||
| 8, // memory level (this is the default value in the docs) | ||
| Z_DEFAULT_STRATEGY // compression strategy | ||
| Z_DEFAULT_COMPRESSION, // compression level |
| @@ -58,7 +60,7 @@ final class ServerInterceptorStateMachineTests: GRPCTestCase { | |||
| stateMachine.interceptedRequestMetadata().assertForward() | |||
| stateMachine.interceptRequestEnd().assertIntercept() | |||
| stateMachine.interceptedRequestEnd().assertForward() | |||
| stateMachine.interceptedRequestEnd().assertCancel() // Can't intercept end twice. | |||
| stateMachine.interceptedRequestEnd().assertCancel() // Can't intercept end twice. | |||
There was a problem hiding this comment.
seems like as all trailing comments have two spaces.
| @@ -207,7 +209,7 @@ class LengthPrefixedMessageReaderTests: GRPCTestCase { | |||
|
|
|||
| // Ensure we can read a message when the rest of the bytes are delivered | |||
| let restOfBytes: [UInt8] = [ | |||
| 0x01, // final byte of message | |||
| 0x01 // final byte of message | |||
There was a problem hiding this comment.
The trailing comma should also not be removed. Another bug worth reporting.
There was a problem hiding this comment.
It's intentional, from https://github.com/apple/swift-format/blob/main/Documentation/Configuration.md:
multiElementCollectionTrailingCommas (boolean): Determines whether multi-element collection literals should have trailing commas. Defaults to true.
Note multi-element. IIRC from digging through issues previously this was to avoid cases where e.g. let x: [Int] could be reformatted as:
let x: [
Int,
]
glbrntt
force-pushed
the
gb-switch-to-swift-format
branch
from
eb56359
to
a48edbc
Compare
Motivation:
SwiftFormat has served us well but doesn't produce
stable formatting over time; the output changes based
on the version used. This can cause churn over time as
the project has to be periodically reformatted.
Having tested multiple versions of swift-format, it's output
is significantly more stable between versions.
Modifications:
Result: