-
Notifications
You must be signed in to change notification settings - Fork 17
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 new rule to add visibility for only method and property inside a … #71
Conversation
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.
Hello @PoulainMaxime that is a very good idea
but this means that if we merge this, any module using php-dev-tools will see their CI go red if they have not prefixed every const with a clear scope, right?
Hi @matks, When you disable the rule for a specific case, it does not check it anymore. This means that whatever you have (with visibility or without it), it won't report anything. |
Then I approve it 😄 |
Hi @matks, if everything is good, is it possible to merge it? Thanks ! |
The PR seems good but, why? 😅 Can you explain why you're disabling the rule to force to set the visibility for const? |
I guess it's because a lot of php developers do not prefix consts by visibility 😅 |
We actually have another reason than habits. :) PS Facebook is reported to fail of early versions of PS 1.7 (reported by @eternoendless earlier this month). This was PHP-Parser (with PREFER_PHP7 flag) in the core unable to read the code after this rule being applied. |
Sorry for being pushy, but is there anything I can do to go on the review? Thanks |
PR looks good to me 👍 @atomiix do you have some last worries or is it OK? |
Thank you @atomiix @PoulainMaxime and @Quetzacoalt91 |
…class