Add new private_swiftui_state_property rule
#4769
Conversation
There was a problem hiding this comment.
Thank you for the contribution @mt00chikin! It's a good starting point.
Are you going to add more SwiftUI attributes to this rule?
It shouldn't be hard to make this rule correctable, that is offer a rewriter to add the private modifier automatically when running swiftlint --fix. What do you think about that?
Source/SwiftLintFramework/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintFramework/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintFramework/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Outdated
Show resolved
Hide resolved
|
I updated the branch based on your feedback and suggestions, they all looked good to me. |
mt00chikin
force-pushed
the
private_swiftui_state_rule
branch
from
42f279d to
dc7dfba
Compare
|
Right now this rule only requires that SwiftUI |
|
@SimplyDanny With the advent of Swift macros and thus |
Convinced. Are you willing to rebase the PR and resolve all conflicts, @mt00chikin? |
|
@SimplyDanny absolutely, I'll try to get that addressed soon and get updates pushed up. |
mt00chikin
force-pushed
the
private_swiftui_state_rule
branch
from
dc7dfba to
6bc0a4f
Compare
Source/SwiftLintBuiltInRules/Rules/Lint/PrivateSwiftUIStatePropertyRuleExamples.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Show resolved
Hide resolved
|
No worries, @SimplyDanny! I appreciate the feedback. Let me know if there's anything else in my latest commit that might have been overlooked. |
mt00chikin
force-pushed
the
private_swiftui_state_rule
branch
from
e07e398 to
5bed5de
Compare
Source/SwiftLintBuiltInRules/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Outdated
Show resolved
Hide resolved
|
There are actually two more things that I've just thought of ... When a declaration is nested inside another declaration implementing struct ContentView: View {
struct OtherStruct {
@State var isPlaying: Bool = false // rule doesn't trigger
}
}the rule doesn't trigger in the nested type. That's fine. However, if it's the other way around as in struct OtherStruct {
struct ContentView: View {
@State var isPlaying: Bool = false // rule doesn't trigger
}
}it also doesn't trigger and that'd be incorrect. It should trigger. We'd have to keep track of the scope we are currently in instead of skipping the type's body completely. Other than that, Apple's documentation you referred to states
which means the rule should not only check for conformance to |
Make private state property rule opt-in Update CHANGELOG.md
…e AttribteListSyntax extension to rule declaration to prevent linter integration warning in project
…pes. Added a new non-triggering example for reference
…new example cases
…d the function to check for the view protocol has been extracted to a function
…inside of an App, Scene, or View type declaration
mt00chikin
force-pushed
the
private_swiftui_state_rule
branch
from
807eae4 to
a8ced28
Compare
|
Let's see if the following example works: struct MyStruct {
struct ContentView: View {
@State ↓var isPlaying: Bool = false
}
@State var isPlaying: Bool = false
} |
|
It doesn't. That's what I get for trying to do this late at night... |
|
Ok, I fundamentally misinterpreted |
…s don't inadvertently affect the state of their parent
Source/SwiftLintBuiltInRules/Rules/Lint/PrivateSwiftUIStatePropertyRule.swift
Outdated
Show resolved
Hide resolved
|
This is going to be merged soon. Thank you for your patience! |
|
Thank you! I appreciate the thorough and patient review feedback. I definitely learned a lot about SwiftSyntax from this experience. |
|
Has this rule been released? I'm trying to use it but I'm getting 'warning: 'private_swiftui_state' is not a valid rule identifier' and the response from 'swiftlint rules' doesn't contain it. Im on version '0.52.4' |
No, it's not yet been released. |
|
I personally think the same rule should apply to |
|
You and Apple both :) Just want to say that this is not a matter of preference whether a |
This is easy to achieve. Anyone willing to open a PR? 😊 |
|
If no one has already started on that work, I can probably find some time to add it this week or next! I actually had considered adding this from the outset, but it just got lost along the way. I'd be happy to add it, I think that it's a good addition to the rule |
Thank you for making the effort. I certainly appreciate it. |
|
FYI, I've opened #5200 to address this feedback and apply the rule to StateObject as well. Feel free to comment on that MR with any additional concerns or requests |
This creates a new opt-in rule that requires that SwiftUI state properties be declared as private. This is inspired by Apple's documentation for State:
This only covers @State and not other SwiftUI property wrappers like @StateObject or @EnvironmentObject.
This MR attempts to address requests from issue: #3173