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

Assign access control by visibility option #144

Merged

Conversation

kitasuke
Copy link
Contributor

It would be nice to use same access control for properties and functions in a struct unless these are not defined in protocol. Since default value of Visibility is set to Internal, we might have public properties and functions in internal struct which looks unnecessary for some case. However, I don't know any background about why Visibility option applies to only struct itself, so it might be better to leave it as it is though.

@allevato
Copy link
Collaborator

This looks reasonable; if a struct is internal, there's no reason for its members to be more visible than that.

Since this PR changes code generation, can you please add commits that regenerate the test protos and reference protos to verify that everything builds and that tests pass after this change? See the documentation in the Makefile. Thanks!

@kientzle
Copy link

This looks good, as long as it passes tests after regenerating.

@kitasuke
Copy link
Contributor Author

Thank you for your feedback! I followed the introduction and committed the test protos and reference protos. I checked that tests passed successfully, but is this correct way?

@tbkka tbkka merged commit fa74ba7 into apple:master Nov 29, 2016
@tbkka
Copy link
Contributor

tbkka commented Nov 29, 2016

Thank you! I'll go ahead and merge this.

@kitasuke kitasuke deleted the adapt_visibility_option_to_property_and_function branch December 1, 2016 07:29
@NachoSoto
Copy link
Contributor

I recently upgraded and all my types default to internal. Which option can I use to control visibility? I tried swift_visibility but no luck.

@NachoSoto
Copy link
Contributor

Never mind, I just found it in the plugin docs. For anyone else looking, you need to set the Visibility option in the plugin, not inside each .proto file.

ahmed-osama-saad pushed a commit to ahmed-osama-saad/swift-protobuf that referenced this pull request Oct 12, 2023
Add proguard rule to lib readme faq section
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

5 participants