Squiz/FunctionDeclarationArgumentSpacing: handle constructor property promotion #792
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…cing after comma" check This refactor has two purposes: * Reduce code duplication. * Prevent introducing even more code duplication when a new error code will be introduced in the next commit.
… comma" vs constructor property promotion
While incorrect spacing after a comma for constructor property promotion parameters would already be flagged and fixed by the sniff, the error message and code were incorrect/unclear.
Given the following test code:
```php
class PropertyPromotionSpacingAfterComma {
public function __construct(private string|int $propA, protected bool $correctSpace, public MyClass $tooMuchSpace, readonly string $noSpace) {}
}
```
Previously the following would be reported:
```
198 | ERROR | [x] Expected 1 space between comma and type hint "MyClass"; 2 found
| | (Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforeHint)
198 | ERROR | [x] Expected 1 space between comma and type hint "string"; 0 found
| | (Squiz.Functions.FunctionDeclarationArgumentSpacing.NoSpaceBeforeHint)
```
Take note of the "type hint" in the message and the "Hint" suffix for the error codes.
With the fix from this commit in place, this will now be reported as follows:
```
198 | ERROR | [x] Expected 1 space between comma and property modifier "public"; 2 found
| | (Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforePropertyModifier)
198 | ERROR | [x] Expected 1 space between comma and property modifier "readonly"; 0 found
| | (Squiz.Functions.FunctionDeclarationArgumentSpacing.NoSpaceBeforePropertyModifier)
```
Includes tests.
…uctor property promotion The spacing after visibility/`readonly` modifiers for constructor property promotion were so far not checked by this sniff. While the `Squiz.WhiteSpace.ScopeKeywordSpacing` sniff will already handle this, that sniff may not be in use in all standards using this sniff. As things were, this sniff was just no longer feature complete for the task this sniff is supposed to handle: spacing of function declaration arguments. This commit adds handling the spacing after modifiers used for constructor property promotion to this sniff. The spacing requirements are aligned with the spacing expectations of the `Squiz.WhiteSpace.ScopeKeywordSpacing` sniff, so the sniffs should not conflict with each other. Additionally, the new checks in this sniff have dedicated error codes, which means that - if there would be a conflict anywhere - the modifier spacing checks within this sniff can easily be turned off. Includes tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Squiz/FunctionDeclarationArgumentSpacing: refactor logic for for "spacing after comma" check
This refactor has two purposes:
Squiz/FunctionDeclarationArgumentSpacing: special case "spacing after comma" vs constructor property promotion
While incorrect spacing after a comma for constructor property promotion parameters would already be flagged and fixed by the sniff, the error message and code were incorrect/unclear.
Given the following test code:
Previously the following would be reported:
Take note of the "type hint" in the message and the "Hint" suffix for the error codes.
With the fix from this commit in place, this will now be reported as follows:
Includes tests.
Squiz/FunctionDeclarationArgumentSpacing: handle modifiers for constructor property promotion
The spacing after visibility/
readonlymodifiers for constructor property promotion were so far not checked by this sniff.While the
Squiz.WhiteSpace.ScopeKeywordSpacingsniff will already handle this, that sniff may not be in use in all standards using this sniff. As things were, this sniff was just no longer feature complete for the task this sniff is supposed to handle: spacing of function declaration arguments.This commit adds handling the spacing after modifiers used for constructor property promotion to this sniff.
The spacing requirements are aligned with the spacing expectations of the
Squiz.WhiteSpace.ScopeKeywordSpacingsniff, so the sniffs should not conflict with each other.Additionally, the new checks in this sniff have dedicated error codes, which means that - if there would be a conflict anywhere - the modifier spacing checks within this sniff can easily be turned off.
Includes tests.
Suggested changelog entry
Changed
SpacingBeforePropertyModifierorNoSpaceBeforePropertyModifiererror codes.Related issues/external references
Loosely related to/ follow up on #620 and #783
Types of changes