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

Remove public from the extension declaration and add it to each declaration in the extension #2602

Merged
merged 1 commit into from
Apr 20, 2024

Conversation

Matejkob
Copy link
Contributor

No description provided.

@Matejkob Matejkob changed the title Add NoAccessLevelOnExtensionDeclaration rule to swift-format Add NoAccessLevelOnExtensionDeclaration rule to swift-format Apr 12, 2024
@ahoppen
Copy link
Collaborator

ahoppen commented Apr 12, 2024

I don’t know with whom I had a conversation about this at some point but I think that private and fileprivate extensions make sense to have. That way if you read private extension, you don’t need to worry about anything leaking out into the rest of the module. public extensions are what I agree can be a source of unintended behavior. Could you maybe:

  • Update this PR to remove all public extensions but keep private and fileprivate
  • File an issue on https://github.com/apple/swift-format to add a rule to only disallows public extension?

@ahoppen ahoppen closed this Apr 12, 2024
@ahoppen ahoppen reopened this Apr 12, 2024
@Matejkob Matejkob force-pushed the NoAccessLevelOnExtensionDeclaration branch from d8fe7ef to 27d6140 Compare April 14, 2024 07:03
@Matejkob Matejkob changed the title Add NoAccessLevelOnExtensionDeclaration rule to swift-format Remove public access level from the extension declaration and add it to each declaration in the extension Apr 14, 2024
@Matejkob Matejkob changed the title Remove public access level from the extension declaration and add it to each declaration in the extension Remove public access level from the extension declaration and add it to each declaration in the extension Apr 14, 2024
@Matejkob Matejkob changed the title Remove public access level from the extension declaration and add it to each declaration in the extension Remove public from the extension declaration and add it to each declaration in the extension Apr 14, 2024
@Matejkob Matejkob force-pushed the NoAccessLevelOnExtensionDeclaration branch from 27d6140 to 0d1d074 Compare April 14, 2024 07:04
@Matejkob Matejkob marked this pull request as ready for review April 14, 2024 07:04
@Matejkob
Copy link
Contributor Author

I don’t know with whom I had a conversation about this at some point but I think that private and fileprivate extensions make sense to have. That way if you read private extension, you don’t need to worry about anything leaking out into the rest of the module. public extensions are what I agree can be a source of unintended behavior. Could you maybe:

  • Update this PR to remove all public extensions but keep private and fileprivate
  • File an issue on https://github.com/apple/swift-format to add a rule to only disallows public extension?

I've removed the swift-format rule and moved the public access level attribute manually.

Copy link
Collaborator

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you 🙏🏽 Just a few formatting suggestions, otherwise looks good to me 👍🏽

Sources/SwiftSyntaxBuilder/SyntaxNodeWithBody.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntaxBuilder/SyntaxNodeWithBody.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntaxBuilder/SyntaxNodeWithBody.swift Outdated Show resolved Hide resolved
@Matejkob Matejkob force-pushed the NoAccessLevelOnExtensionDeclaration branch from 0d1d074 to ce8b9cd Compare April 19, 2024 17:17
Copy link
Collaborator

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks 👍🏽

@ahoppen
Copy link
Collaborator

ahoppen commented Apr 20, 2024

@swift-ci Please test

@ahoppen
Copy link
Collaborator

ahoppen commented Apr 20, 2024

@swift-ci Please test Windows

@ahoppen ahoppen enabled auto-merge April 20, 2024 04:50
@ahoppen
Copy link
Collaborator

ahoppen commented Apr 20, 2024

Could you also create a cherry-pick PR of this to the release/6.0 branch? I think it could make future cherry-picks easier because there are fewer conflicts.

@ahoppen ahoppen merged commit 5bd6384 into apple:main Apr 20, 2024
3 checks passed
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.

None yet

2 participants