Squiz.WhiteSpace.FunctionSpacing last method no space

[ofbeaton]

It would be nice if Squiz.WhiteSpace.FunctionSpacing.After would not require a blank line after a method if it is the last method in a class.

class A 
{
  public function method1()
  {
  }

  public function method2()
  {
  }

  public function method2()
  {
  }
}

This would be much nicer than enforcing a space after the last method. You could switch it to enforce a space before a method, but then you'd run into the same problem but at the top (assuming no properties).

[aik099]

In fact I think it should also be checking for empty line above function declaration. Currently other sniff checks that, but only if class has any properties.

[techi602]

Sadly FunctionSpacing is useless without ability to skip first and last method because it violates PSR-2 (at least the example in PSR-2 example 1.1)

So this rule

    <rule ref="Squiz.WhiteSpace.FunctionSpacing" >
        <properties>
            <property name="spacing" value="1" />
        </properties>
    </rule>

violates http://www.php-fig.org/psr/psr-2/#1-1-example

<?php
namespace Vendor\Package;

use FooInterface;
use BarClass as Bar;
use OtherVendor\OtherPackage\BazClass;

class Foo extends Bar implements FooInterface
{
    public function sampleFunction($a, $b = null)
    {
        if ($a === $b) {
            bar();
        } elseif ($a > $b) {
            $foo->bar($arg1);
        } else {
            BazClass::bar($arg2, $arg3);
        }
    }

    final public static function bar()
    {
        // method body
    }
}

@TomasVotruba @OndraM

Do you guys have some sort of control for function line spacing in your projects?

Thanks

This PR is open for more than a year :cry:

[TomasVotruba]

I am not sure, but it think there is some hidden dependency on other sniff, that takes care of this issue. Because I use whole "psr2" standard and I've never came across this.

[techi602]

@TomasVotruba the problem with default PSR-2 ruleset is that it does not cover the spaces between methods at all https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/PSR2/ruleset.xml

So you can have 5 spaces between methods without warning :/

I guess there are not many people having this issue but my junior developers are generating the random amount of spaces between methods in almost every PR :smile:

[aik099]

I see no problem with adding 2 more settings to existing sniff that are:

1. on by default
2. tell if empty lines (number of lines is controlled by existing setting) should be present at the start (one setting) and at the end (another setting)

Please send a PR.

[OndraM]

@techi602 FYI I just checked our setup, and we do cover this rule using extra_empty_lines check from php-cs-fixer.

[TomasVotruba]

@OndraM I recall now, that is how I solved that :).

[gsherwood]

Sadly FunctionSpacing is useless without ability to skip first and last method because it violates PSR-2

It does not violate PSR-2 because PSR-2 does not have any rules about method, function, or property spacing. If I could use the examples to infer all sorts of rules, my life would be a lot easier. But I've been told by the PHP-FIG that I can't. So I don't.

the problem with default PSR-2 ruleset is that it does not cover the spaces between methods at all So you can have 5 spaces between methods without warning :/

Yep. PSR-2 (the written standard) allows that. It also allows a range of other bad code formatting because it really isn't that strict.

[bcalik]

Any progress on this?

[oojacoboo]

Bump. 👍

[oojacoboo]

@OndraM are you using that rule directly with phpcs, or are you using the symfony cs fixer in addition? If directly with phpcs, any info on how to go about that? Thanks.

[OndraM]

@oojacoboo I'm using both of these php code style fixers. Each of them covers different set of rules...

[oojacoboo]

@OndraM thanks. Ugh, more bloat :(

Going to have to find another way. I really don't need another runner/linter to solve these issues.

[OndraM]

@oojacoboo I think you will always need combination of multiple tools. Codestyle with phpcs and php-cs-fixer, static analysis with eg. phpstan, linting... https://github.com/phpro/grumphp is a nice way how to combine many of these tools, btw.

[oojacoboo]

@OndraM you very well may be right. It's just a shame how fragmented things are. And piecing together all of this isn't exactly clean. Thanks for the comments - much appreciated.

[oojacoboo]

Okay, so I was able to solve this issue using the rules from, https://github.com/slevomat/coding-standard. Specifically the rule, SlevomatCodingStandard.Types.EmptyLinesAroundTypeBraces.

Hope this helps anyone else that might come across this issue.

[gsherwood]

Adding a spacingAfterLast property isn't too hard, but adding a spacingBeforeFirst requires a decision. If you've got a class with properties:

class MyClass
{

    protected $tag = '';

    protected $tag = '';

    /**
     * Function comment.
     *
     * @return boolean
     */
    function func1() {

    }//end func1()

}//end class

Should you apply the spacing property (e.g., 2 blank lines) or the spacingBeforeFirst property (e.g., no blank lines)?

It would be pretty weird to see code like this:

class MyClass
{

    protected $tag = '';

    protected $tag = '';
    /**
     * Function comment.
     *
     * @return boolean
     */
    function func1() {

    }//end func1()
}//end class

Where the method is considered the first/last/only function in the class and so has no padding around it.

Because of this, I think I'll only apply the spacingBeforeFirst and spacingAfterLast property values when a function is the first/last item in the class.

If anyone feels strongly against this, please speak up.

[gsherwood]

I've committed the new props, but with a BC break. I'm still working on a change to try and remove the need for the BC break, but it can be tested/reviewed as-is if anyone wants to.

[jrfnl]

@gsherwood Regarding the BC break - what about this:

Let the default for the new properties be null. Then at the start of the sniff, check for each of the new properties if they are set, if not, let them fall back to the value of the $spacing property.

Would that work ?

[gsherwood]

I'm still working on a change to try and remove the need for the BC break,

That didn't take as long as I thought, so it's done now. Both the new spacingBeforeFirst and spacingAfterLast properties will use whatever value has been set for the spacing property if you don't set a value for them. This means that all existing rulesets will continue to work as normal.

Docs for the new properties are here: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties#squizwhitespacefunctionspacing

I'll leave this issue open for a while to gather any feedback.

[ofbeaton]

Thanks for resolving this 2 1/2 year old issue! Can't wait to use it in the next version. We can re-open if for some reason it's broken or not behaving correctly.

[gsherwood]

Re-opening so it doesn't disappear from my 3.3.0 roadmap.

[gsherwood]

Let the default for the new properties be null. Then at the start of the sniff, check for each of the new properties if they are set, if not, let them fall back to the value of the $spacing property.

Sorry, I missed this comment, but I think you had the exact same idea as I did in the end. I just wasn't sure if I'd be able to support both ruleset values and phpcs:set values, but it ended up working fine.

[gsherwood]

Closing now as there have been no new comments.