Skip to content

PEAR/ScopeClosingBrace: fix fixer conflict #1813

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Dec 26, 2017

Another small PR to fix a fixer conflict between the PEAR.WhiteSpace.ScopeClosingBrace and the PEAR.WhiteSpace.ScopeIndent (extended from Generic.WhiteSpace.ScopeIndent) sniffs.

If a case or default statement uses braces, the scope indent and scope closing brace sniffs would fight over where the brace should be placed.

The unit test file already contains a test case highlighting the issue.


To reproduce the issue, run the following command on the master branch:
phpcbf -p -s -vv ./src/Standards/PEAR/Tests/WhiteSpace/ScopeClosingBraceUnitTest.inc --standard=PEAR

Note: this change does change the behaviour of the sniff in that particular case. If that's undesired, another solution will need to be found.

Another small PR to fix a fixer conflict between the `PEAR.WhiteSpace.ScopeClosingBrace` and the `PEAR.WhiteSpace.ScopeIndent` (extended from `Generic.WhiteSpace.ScopeIndent`) sniffs.

If a `case` or `default` statement uses braces, the scope indent and scope closing brace would fight over where the brace should be placed.

The unit test file already contains a test case highlighting the issue.

To reproduce the issue, run the following command on the `master` branch:
`phpcbf -p -s -vv ./src/Standards/PEAR/Tests/WhiteSpace/ScopeClosingBraceUnitTest.inc --standard=PEAR`
@gsherwood
Copy link
Member

I don't like the change of behaviour for PEAR.WhiteSpace.ScopeClosingBrace.BreakIndent with this, but there is a clear conflict here.

I think the only way to fix this is figure out how to change PEAR.WhiteSpace.ScopeIndent to not report errors for the closing brace of CASE and DEFAULT statements. But I don't really want to do that with a new sniff property because it's pretty specific, so that might mean (finally) refactoring the main scope indent sniff so that some of it can be overridden in the PEAR sniff.

@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 16, 2018

that might mean (finally) refactoring the main scope indent sniff so that some of it can be overridden in the PEAR sniff.

@gsherwood Yes, that's one I haven't dared to touch yet, but I'm seeing more conflicts which involve that sniff. I still need to find the time to dig in deeper to come up with good test cases though.
Will report back when I have.

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