Skip to content

Squiz fixer conflict in file with side effects #2033

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

Closed
jrfnl opened this issue May 28, 2018 · 4 comments
Closed

Squiz fixer conflict in file with side effects #2033

jrfnl opened this issue May 28, 2018 · 4 comments
Milestone

Comments

@jrfnl
Copy link
Contributor

jrfnl commented May 28, 2018

The following code sample will give a fixer conflict when the Squiz ruleset is run against it as the Squiz.Classes.ClassDeclaration sniff demands exactly one blank line after a class, while the Squiz.WhiteSpace.FunctionSpacing sniff demands exactly two blank lines before a function.

class MyClass2
{
    var $x;
}

/** ************************************************************************
 * Example with no errors.
 **************************************************************************/
function example() {}

This can be prevented from either sniff by checking either that the next non-empty token is a function keyword (ClassDeclaration) or that the previous non-empty token is a close brace of an OO construct (FunctionSpacing) and bowing out if that's the case.

@gsherwood I'm not sure which of the two sniffs should have precedence, i.e. which sniff should be adjusted ? Guidance appreciated.
I'm happy to prepare the PR once it's clear what the desired fix should be.

Also: there are a number of unintentional parse errors in test files where global functions are prefixed with a visibility keyword. Those would need to be fixed too, as otherwise the fixer conflict would still show (previous/next non-empty would be the visibility keyword breaking the fix).

@gsherwood gsherwood added this to the 3.3.1 milestone Jun 7, 2018
@gsherwood
Copy link
Member

@gsherwood I'm not sure which of the two sniffs should have precedence, i.e. which sniff should be adjusted ? Guidance appreciated.

I think I'd get ClassDeclaration to not throw an error if the class is followed by a function. But it probably doesn't matter given this sort of code is unlikely to be encountered anyway, so whatever is easier is fine I think.

gsherwood added a commit that referenced this issue Jun 22, 2018
@gsherwood
Copy link
Member

I ended up going with ClassDeclaration not throwing an error when followed by a function. Haven't removed the parse errors from test files yet though.

@gsherwood
Copy link
Member

I ran those two sniffs over the test files and they produced fixed content without issue, so I'm not sure if there are any conflicts in the test files. If you find some though, please let me know and I'll fix.

@jrfnl
Copy link
Contributor Author

jrfnl commented Jun 22, 2018

@gsherwood Thank you! I hadn't gotten round to doing this myself yet.

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

No branches or pull requests

2 participants