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

Squiz.Functions.FunctionDeclarationArgumentSpacing fixer removes comment between parens when no args #2006

Closed
hchow77 opened this issue Apr 25, 2018 · 5 comments
Milestone

Comments

@hchow77
Copy link

hchow77 commented Apr 25, 2018

Interesting case I hit with SpacingBetween.

I have this code where in place of the parameters, there's a comment there that is meant to show that an arbitrary number of parameters can be passed into the function, as at a later point, it calls into another function with the result of func_get_args():

<?php

class Foo {
    public function bar(/*...*/) {
        return call_user_func_array('baz', func_get_args());
    }
}

When running Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBetween on this, I get an interesting message with PHPCS:
"Expected 0 spaces between brackets of function declaration; 0 found"

When running that rule with the PHPCBF, it ends up removing the comment so it's just bar().

So beside the argument on whether the comment itself should or should not be there, I have some thoughts about this:

  • As this is a spacing rule, I would expect the results of fixes to be only whitespace changes, not changes to any non-whitespace, comments included.
  • The message is a little odd, given that 0 expected and 0 found sounds like it should not be an error. If anything, this case may deserve a separate message type that is more specific to the change that we expect it to make.

Thanks for the consideration.

@bobbykjack
Copy link

As an aside, you may want to document the variable arguments explicitly. Further details in this stackoverflow question.

@hchow77
Copy link
Author

hchow77 commented May 1, 2018

Right, I recognize there's other stuff I can do with this code, but I feel like both of my thoughts are still valid.

@gsherwood
Copy link
Member

As this is a spacing rule, I would expect the results of fixes to be only whitespace changes, not changes to any non-whitespace, comments included.

Yes, it should only be making whitespace changes. In cases like this encountered in other sniffs, the sniff will actually report that the error is not fixable.

The message is a little odd, given that 0 expected and 0 found sounds like it should not be an error. If anything, this case may deserve a separate message type that is more specific to the change that we expect it to make.

The sniff detected that the function has no arguments, but then assumes the content between the parenthesis must be spaces. It doesn't really matter if it checks spaces after the opener or before the closer (it used the closer) because it should be the same amount of whitespace. But the comment is tripping it up and it finds no spaces before the closer, hence the weird error message.

I don't think the sniff should actually be throwing an error in this case as it is only trying to stop code like foo( ) from being written. If this style of code is not desired, another sniff could be written to ban comments from inside function argument declarations.

@gsherwood gsherwood added this to the 3.3.0 milestone May 2, 2018
@gsherwood gsherwood changed the title Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBetween auto-corrects by removing comment within args Squiz.Functions.FunctionDeclarationArgumentSpacing fixer removes comment between parens when no args May 2, 2018
gsherwood added a commit that referenced this issue May 2, 2018
…fixer removes comment between parens when no args
@gsherwood
Copy link
Member

The change I've made treats bar(/*...*/) like a non-empty definition now, where it previously saw it as empty. So the sniff will enforce the correct number of spaces around the parenthesis instead of reporting that there should be no spaces. If the number of spaces is wrong, that will be corrected when fixing and the comment will be left alone.

Thanks for reporting this.

@hchow77
Copy link
Author

hchow77 commented May 2, 2018

And thank you for getting in a fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants