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 package ACL #5

Closed
wants to merge 1 commit into from
Closed

Add package ACL #5

wants to merge 1 commit into from

Conversation

davdroman
Copy link
Contributor

No description provided.

@gohanlon gohanlon mentioned this pull request Nov 14, 2023
2 tasks
@gohanlon
Copy link
Owner

gohanlon commented Nov 14, 2023

Awesome, thanks for the PR!

Pretty straightforward. The only thing I want to consider is the default behavior of optionalsDefaultNil when providing an initializer with package access. In the meantime, I added a tracking issue (#6).

@davdroman
Copy link
Contributor Author

davdroman commented Nov 14, 2023

My 2 cents, personally I've always found the behavior of the built-in synthesized memberwise inits in Swift annoying and unpredictable when it comes to defaulting optional params to nil — I talked about it here.

And there has even been a popular forum discussion advocating for the removal of implicit nil assignments altogether.

So, if it were up to me, optionalsDefaultNil wouldn't even be an option, but if it had to be, it would be always false across all access levels unless the user opted in explicitly.

@gohanlon
Copy link
Owner

I too lament that Swift provides initializers defaulting optionals to nil. A design tenet of MemberwiseInit has been to strive to be a pure superset of Swift's memberwise initializer, and this approach has largely served it well. But in this case, I'm now convinced that it makes sense to deviate from Swift 5 and always set optionalsDefaultNil to false, regardless of access level.

I've created #7 to track and discuss that!

I'm leaning towards keeping optionalsDefaultNil as an option to allow for closer parity with Swift's behavior.

@gohanlon gohanlon self-assigned this Nov 14, 2023
gohanlon added a commit that referenced this pull request Nov 14, 2023
This commit builds upon the work done in PR #5 by David Roman, with the
following changes:

* Don't default to `optionalsDefaultNil` for `package` access level
* Add tests

Co-authored-by: Galen O’Hanlon <[email protected]>
@gohanlon
Copy link
Owner

Thanks for your contribution! I've merged your changes with some adjustments into PR #9 with some minor tweaks and tests. Closing this PR as we move forward with PR #9.

@gohanlon gohanlon closed this Nov 14, 2023
gohanlon added a commit that referenced this pull request Nov 14, 2023
Fixes #6.

This commit builds upon the work done in PR #5 by David Roman, with the
following changes:

* Don't default to `optionalsDefaultNil` for `package` access level
* Add tests

Co-authored-by: Galen O’Hanlon <[email protected]>
gohanlon added a commit that referenced this pull request Nov 14, 2023
Fixes #6.

This commit builds upon the work done in PR #5 by David Roman, with the
following changes:

* Don't default to `optionalsDefaultNil` for `package` access level
* Add tests

Co-authored-by: Galen O’Hanlon <[email protected]>
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.

2 participants