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

completing access modifiers example, by also adding open's default members access level. #208

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

deya-eldeen
Copy link

readers might be in doubt about the open access modifier, especially internal members, adding this part will explain this more and remove doubts.

we can remove some parts of it for brevity, but this line is important

var someInternalProperty = 0 // implicitly internal class member

readers might be in doubt about the open access modifier, especially internal members, adding this part will explain this much more.
@amartini51
Copy link
Member

Thanks for starting this discussion! A couple issues that make this change a bit challenging to review:

  • The commit message should describe what you changed and why, not just restate which file contains the changes. (If you're editing on GitHub.com you need to replace the default message it gives you.)
  • The pull request should describe the problem in the current text. In this case, what are developers confused about? What incorrect conclusion are they likely to arrive at?
  • The pull request should describe why this solution resolves that problem. For example, here, why is it sufficient to add examples to the code? What about the prose nearby — does that need updates?

For more detailed guidance, please see CONTRIBUTING in this repository.

@amartini51 amartini51 marked this pull request as draft November 15, 2023 21:51
@amartini51
Copy link
Member

@deya-eldeen Is this a change you're still interested in making or advocating for? Or can I close this PR as "not to be merged"?

@deya-eldeen
Copy link
Author

deya-eldeen commented Jun 25, 2024 via email

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