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/FunctionDeclarationArgumentSpacing: make the reference operator spacing configurable #3900

Closed
wants to merge 1 commit into from
Closed

Squiz/FunctionDeclarationArgumentSpacing: make the reference operator spacing configurable #3900

wants to merge 1 commit into from

Conversation

darrenedale
Copy link

Description

Enables the amount of whitespace between the reference operator and the argument name in function declarations to be defined by a property in XML config. The existing sniff fixes this at 0 spaces, meaning function declarations with by-reference arguments must look like this:

function do_something(string &$withThis): void;

My coding style follows PSR12, with a few modifications, one of which is to have a space between the reference operator and the argument name:

function do_something(string & $withThis): void;

Since the PSR12 standard imports Standards\Squizz\Sniffs\Functions\FunctionDeclarationArgumentSpacingSniff, and since at present this sniff has a strict requirement of 0 spaces between the reference operator and the argument name, it's not possible to base my standard off PSR12 and make this customisation. This PR resolves that by making the amount of space between the reference operator and the argument name configurable in the XML file, thus:

<rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing">
    <properties>
        <property name="requiredSpacesAfterReferenceOperator" value="1"/>
    </properties>
</rule>

The feature has been implemented similarly to other properties that are configurable in this sniff. Notably, the property controlling the required number of spaces after the reference operator defaults to 0, ensuring that where no customisation is present for this property for this sniff in the configuration file, the sniff continues to behave as it does currently. Therefore existing workflows using this sniff will be unaffected.

Suggested changelog entry

Allow the required spacing between the reference operator and the argument name in function declarations to be customised in configuration files for the Squizz standard's FunctionDeclarationArgumentSpacing sniff.

Related issues/external references

Fixes #

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes. *
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.
  • [Required for new files] I have added any new files to the package.xml file.

* I've checked the test and it doesn't look like it's possible to test the sniff with alternate configurations. I'm
happy to write one if I've missed how this is done.

…pacingSniff

Enable the amount of whitespace between the reference operator and the argument name in function declarations to be defined by a property in XML config.
@jrfnl jrfnl changed the title Configurable reference operator spacing in Standards\Squizz\Sniffs\Functions\FunctionDeclarationArgumentSpacingSniff Squiz/FunctionDeclarationArgumentSpacing: make the reference operator spacing configurable Oct 14, 2023
@jrfnl
Copy link
Contributor

jrfnl commented Oct 14, 2023

@darrenedale I've never seen anyone else ask for this and it goes against industry standards like PSR12, so I'm not sure this feature should be accepted without validation that this is something that more than one person intends to use.

I'm leaving this PR open for now to see if more people are interested in this feature.

Please be aware that as things are, the PR is not in a mergable state as there are no tests covering the new feature included.

@darrenedale
Copy link
Author

OK, I'll find a different solution.

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

Successfully merging this pull request may close these issues.

2 participants