Skip to content
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

Add SwiftUI Property declaration to sub section ordering list #282

Merged
merged 8 commits into from
Aug 1, 2024

Conversation

miguel-jimenez-0529
Copy link
Contributor

@miguel-jimenez-0529 miguel-jimenez-0529 commented Jul 22, 2024

Summary

After merging nicklockwood/SwiftFormat#1747 and #280, this PR adds the SwiftUI property declaration ordering rule to the style guide.

Reasoning

SwiftUI Properties are a special type of property that lives inside SwiftUI views. These views conform to the DynamicProperty protocol and cause the a view's body to re-compute. Given this common functionality and also a similar syntax, it is preferred to group them together.

// WRONG

struct CustomSlider: View {

  // MARK: Internal

  var body: some View {
    ...
  }

  // MARK: Private

  @Binding private var value: Value
  private let range: ClosedRange<Double>
  @Environment(\.sliderStyle) private var style
  private let step: Double.Stride
  @Environment(\.layoutDirection) private var layoutDirection
}

// RIGHT

  struct CustomSlider: View {
  
  // MARK: Internal

  var body: some View {
    ...
  }

  // MARK: Private

  @Environment(\.sliderStyle) private var style
  @Environment(\.layoutDirection) private var layoutDirection
  @Binding private var value: Value
  
  private let range: ClosedRange<Double>
  private let step: Double.Stride
}

@miguel-jimenez-0529 miguel-jimenez-0529 marked this pull request as ready for review July 22, 2024 23:24
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -28,7 +28,7 @@
--enumthreshold 20 # organizeDeclarations
--organizetypes class,struct,enum,extension,actor # organizeDeclarations
--visibilityorder beforeMarks,instanceLifecycle,open,public,package,internal,fileprivate,private # organizeDeclarations
--typeorder nestedType,staticProperty,staticPropertyWithBody,classPropertyWithBody,instanceProperty,instancePropertyWithBody,staticMethod,classMethod,instanceMethod # organizeDeclarations
--typeorder nestedType,staticProperty,staticPropertyWithBody,classPropertyWithBody,swiftUIPropertyWrapper,instanceProperty,instancePropertyWithBody,staticMethod,classMethod,instanceMethod # organizeDeclarations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
* Class properties
* Static property with body
* Class properties with body
* SwiftUI dynamic properties (@State, @Environment, @Binding, etc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miguel-jimenez-0529 what do you think about generalizing this section to be any property with a property wrapper? I'm curious about the reason to scope this specifically to SwiftUI property wrappers. cc: @calda

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me and Cal had the same conversation. Our conclusion was that there are @ properties like @objc that are not property wrappers. Also a property wrapper doesn't have a common semantic meaning, examples of those are @Referenced, @UniBindable, @Atomic, @Resilient. So grouping all property wrappers when we don't know upfront their functionality was a bit opinionated. Engs may have a better idea on where to place them.
This is different for SwiftUI property wrappers since they all conform to the DynamicProperty protocol and have a common semantic meaning in the sense that they interact with the UI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. Thanks for explaining!

Copy link
Member

@calda calda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @miguel-jimenez-0529! Lots of support for this change.

@calda calda enabled auto-merge (squash) August 1, 2024 16:40
@calda calda merged commit 77b4f2d into master Aug 1, 2024
5 checks passed
@calda calda deleted the mj-swiftui-property-ordering branch August 1, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants