-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Minor fixes to the 'getting started' page #6179
Conversation
biggianteye
commented
Dec 19, 2016
- Use consistent visibility for some fields.
- Fixing path of yaml file.
- Minor grammar changes.
@@ -717,8 +717,8 @@ the bi-directional reference: | |||
{ | |||
// ... (previous code) | |||
|
|||
private $reportedBugs = null; | |||
private $assignedBugs = null; | |||
protected $reportedBugs = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should stay private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why? All other instance variables are marked as protected
. Why should these two be private
?
More importantly, they were declared as protected
when first added to the User
class. If there is a legitimate need for the change to the visibility, let me know and I'll incorporate it into the section where the visibility changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have everything as private
around the code, but if you think that protected
is better for consistency (for now), I can merge the PR as-is.
Should generally strive for private
in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not trying to advocate one over the other (though if I were, I'd go for private). I'm just trying to remove any confusion caused by modifiers changing half way through introductory documentation without any explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, merging as-is then :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
private $reportedBugs = null; | ||
private $assignedBugs = null; | ||
protected $reportedBugs = null; | ||
protected $assignedBugs = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should stay private