nicklockwood / SwiftFormat

A command-line tool and Xcode Extension for formatting Swift code
MIT License
7.67k stars 623 forks source link

Add option to wrap complex attributes with arguments differently than simple attributes without arguments #1591

Closed calda closed 7 months ago

calda commented 7 months ago

This PR adds a --complexattrs option to support wrapping complex attributes (attributes with arguments) differently from simple attributes (without arguments).

We want to use --storedvarattrs same-line for common attributes like @State and @objc:

@State private var foo: Foo
@objc private var bar: Bar

but the most idomatic usage of longer attributes (typically those with arguments, like @available or custom DSLs like Swift argument parser) is to leave them on the previous line:

// Uncommon:
@available(*, deprecated, message: "Deprecated!") var foo: WrappedType
@Option(help: "The absolute path to the SwiftFormat config file") var swiftFormatConfig: String

// More common:
@available(*, deprecated, message: "Deprecated!") 
var foo: WrappedType

@Option(help: "The absolute path to the SwiftFormat config file") 
var swiftFormatConfig: String

This is now supported with --storedvarattrs same-line --complexattrs prev-line.


This does pose a question about how to handle the SwiftUI @Environment attribute. I find that idiomatic usage of this attribute specifically is to leave it on the same line as the property to match other SwiftUI property wrappers, like:

@State private var foo: Foo
@Environment(\.barFromEnvironment) var bar

By default --complexattrs prev-line would apply to this. To support avoiding that this PR also adds a --noncomplexattrs option that lets you exclude individual attributes from the --complexattrs option. So in our case, we would use --noncomplexattrs @Environment.

Since I've only ever seen @Environment written in this way (and not wrapped). I think it makes sense to include --noncomplexattrs @Environment as the default behavior.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ead08ee) 95.05% compared to head (3a0bc4d) 95.08%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1591 +/- ## =========================================== + Coverage 95.05% 95.08% +0.03% =========================================== Files 20 20 Lines 22060 22084 +24 =========================================== + Hits 20969 20999 +30 + Misses 1091 1085 -6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nicklockwood commented 7 months ago

@calda I wonder if rather than the complex/noncomplex distinction, it might might make sense to have a width threshold? That would also allow for scenarios where there are multiple noncomplex attributes, or custom property wrappers with long names e.g.

@objc @MainThread @SomeVeryLongPropertyWrapper
public var myProperty: String
calda commented 7 months ago

Thanks for the suggestion @nicklockwood. I just looked at some different example cases of this, including a more in-depth look at the patterns in our codebase. Here are some examples that I'm considering and hoping to account for based on usage I tend to see, plus my own personal aesthetic preferences:

  1. @available attributes should always be on the previous line.
// Good
@available(*, deprecated) 
public var deprecatedFoo: Foo

@available(iOS 17, *) 
public var iOS17Foo: Foo

// Bad
@available(*, deprecated) public var deprecatedFoo: Foo
@available(iOS 17, *) public var iOS17Foo: Foo

The shortest @available attribute I can think of (@available(iOS 9, *), etc) is 19 characters.

  1. SwiftUI view attributes should basically always be on the same line. In all code examples I can find online, and basically everywhere in our codebase, idiomatic usage of these attributes is to put them on the same line.
// Good
@State var foo: Foo
@Published var publishedFoo: Foo
@ObservedObject var observedFoo: Foo
@EnvironmentObject var environmentFoo: Foo
@Environment(\.dismiss) private var dismiss

// This one is a lot more uncommon, but is still always written on a single line in sample code I see online.
@UIApplicationDelegateAdaptor private var appDelegate: MyAppDelegate

// Bad
@State 
var foo: Foo

@Published
var publishedFoo: Foo

@ObservedObject 
var observedFoo: Foo

@EnvironmentObject 
var environmentFoo: Foo

@Environment(\.dismiss) 
private var dismiss

@UIApplicationDelegateAdaptor
private var appDelegate: MyAppDelegate

Regarding always wrapping attributes beyond some length threshold, I find that attributes with arguments are basically always wrapped, even if the attribute itself is very short:

// Most common
@available(iOS 17, *) // 20 chars
public var foo: Foo

@Option(name: .long) // From Swift Argument Parser. 19 chars.
var configPath: String

@Tweak(name: "width") // From an internal library that lets you customize properties at runtime. 20 chars.
var width: Double

// Less common
@available(iOS 17, *) public var foo: Foo
@Option(name: .long) var configPath: String
@Tweak(name: "width") var width: Double

Those examples are all 20 characters or fewer. On the other hand, simple attributes with similar lengths but without arguments tend to be left on a single line:

// Most common
@EnvironmentObject var environmentFoo: Foo // 17 chars
@MyEnvironmentObject var environmentFoo: Foo // 19 chars
@CustomEnvironmentObject var environmentFoo: Foo // 25 chars
@UIApplicationDelegateAdaptor private var appDelegate: MyAppDelegate // 28 chars

// Less common
@EnvironmentObject
var environmentFoo: Foo

@MyEnvironmentObject
var environmentFoo: Foo

@CustomEnvironmentObject
var environmentFoo: Foo

@UIApplicationDelegateAdaptor
private var appDelegate: MyAppDelegate

@Environment being an exception to the "wrap complex attributes" rule is a bit unfortunate, but it would also have to be an exception to any "wrap long attributes" rule since I basically never see this wrapped (even up to 40-50+ chars).

calda commented 7 months ago

So, that being said:

I think it can definitely make sense to support a max width option for attributes, but in our case I think we would want the max width to be at least 30 chars (maybe more), so that wouldn't completely replace the need to distinguish between attributes with and without arguments.

calda commented 7 months ago

Didn't notice I had pushed changes with a failing test case. Just fixed the test so CI should pass now.