PHPCSStandards/PHP_CodeSniffer

Class bracing sniffs with PER 2.0

Opened this issue · 3 comments

rikvdh commented

As I'm working on the implementation of PER2.0 I'm facing some issues with some sniffs.

In the first introductory of section 4 of PER 2.0 it states:

If class contains no additional declarations (such as an exception that exists only to extend another exception with a new type), then the body of the class SHOULD be abbreviated as {} and placed on the same line as the previous symbol, separated by a space. For example:

class MyException extends \RuntimeException {}

For this this issue, specifically: PSR2.Classes.ClassDeclaration.OpenBraceNewLine and Squiz.WhiteSpace.ScopeClosingBrace.ContentBefore have an effect.

PSR2.Classes.ClassDeclaration.OpenBraceNewLine

This is for PER 2.0 'not always' valid anymore. When the closing brace is adjacent to the opening brace, it needs to not be on a newline. To check this in this sniff might be odd since it is not related to PSR2. But I would like some option to 'disable' specific cases for this error.

We could also make a new sniff where we inherit this one and extend it for PER2.0?
We should discuss the desired solution for this.

Squiz.WhiteSpace.ScopeClosingBrace.ContentBefore

This is a difficult sniff for PER 2.0. Since closing braces do not need to be on a line by itself when the opening-brace is adjacent to the left AND for only functions, methods and classes.
The easiest fix for PER2.0 might be to have an option to this sniff to disable this error when a opening brace is adjacent on the left. The openbrace sniffs might catch the cases where the 'closing brace is not allowed there' and then we omit this rule for these situations.

@jrfnl Combining 2 sniffs in one issue feels a bit odd I think, but it is related to one section of PER 2.0. Let me know if you rather like '1 sniff discussion' per issue.

jrfnl commented

@rikvdh Would you mind adding the specific rule in PER which you're trying to address ? That should make it more straight forward to figure out what the best solution direction will be.

rikvdh commented

Thank you. I've updated the issue. While thinking about this, having a configuration option for both sniffs to disable/enable this behavior is the most compatible way I think and is not very hard to implement.

jrfnl commented

While thinking about this, having a configuration option for both sniffs to disable/enable this behavior is the most compatible way I think and is not very hard to implement.

@rikvdh My current thinking on this is as follows:

  • This shouldn't be a configuration option, but a property which can be added to individual sniffs.
  • The property should in principle only be added to sniffs where the rules of PSR-12 (or PSR-2) would conflict with the rules in PSR-PER.
  • The property should always have the same name and default value (and documented string type and visibility) in all sniffs which get this property, i.e. public $per_compatible = '1.0'.
  • The default value should be 1.0 as that's basically PSR-12. That also means that the default behaviour of sniffs will not change. It is an opt-in option, meaning no breakage for existing users.
  • Considering rules in PSR-PER may be different per version - and different versions may conflict with each other -, I think making it a version string is the most future-proof way of setting this up.
  • Within the sniff, different rules can then be enforced based on that property: 
    if (version_compare( $this->per_compatible, '2.0', '>=') === true) {
    // Do something PER 2.0 specific
} else {
    // Enforce the PSR-12 rule
}
  • By naming the property the same everywhere, it can be set at a ruleset level (particularly useful for the PER rulesets) and would then automatically be applied to all sniffs which support the property: 
    <rule ref="PSR12">
    <property name="per_compatible" value="2.0"/>
</rule>

Does that align with what you had in mind ?