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

Adopt SE-409 #134

Closed
wants to merge 3 commits into from
Closed

Adopt SE-409 #134

wants to merge 3 commits into from

Conversation

czechboy0
Copy link
Contributor

@czechboy0 czechboy0 commented Dec 17, 2024

Motivation

Modifications

Added explicitly access modifiers on imports.

Result

Made it more explicit which types we're importing publicly and are used in our own API.

Test Plan

Tests passed, will let CI ensure this works in all our supported Swift versions.

@czechboy0 czechboy0 changed the title Adopt SE-409 and SE-444 Adopt SE-409 Dec 17, 2024
@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Dec 17, 2024
@@ -12,7 +12,7 @@
//
//===----------------------------------------------------------------------===//
import Foundation
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this API breaking? You were previously importing Foundation as public but with the changed flag it is now imported as internal. Since imports are leaky in Swift this would be API breaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The module still publicly imports Foundation, just in other files. So the module as a whole doesn't remove the public import.

But it's kind of moot - seems for packages that still need to support 5.x, making both 5.x and 6.x compilers warning free with explicit access modifiers on imports would require a ton of #if guards, so I'm punting on this for now.

@czechboy0 czechboy0 closed this Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants