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

MethodSignatureSniffs: Add included and exluded patters #854

Merged
merged 4 commits into from
Jan 29, 2020
Merged

MethodSignatureSniffs: Add included and exluded patters #854

merged 4 commits into from
Jan 29, 2020

Conversation

RiKap
Copy link
Contributor

@RiKap RiKap commented Jan 21, 2020

Hey.
I added includedMethodPatterns and excludedMethodPatterns property for RequireSingleLineMethodSignatureSniff and RequireMultiLineMethodSignatureSniff

includedMethodPatterns:
It will allow you to set methods which you would like to check by these two sniffs

excludedMethodPatterns:
It will allow you to set methods which you would like to ignore by these two sniffs


Sometimes it is good to enforce RequireMultiLineMethodSignatureSniff just for one method, for example __construct.

Another benefit of this change is that you are able to use these two sniffs together.
It will force RequireMultiLineMethodSignature for __construct and RequireSignleLineMethodSignature for all other methods.

<rule ref="SlevomatCodingStandard.Classes.RequireMultiLineMethodSignature">
	<properties>
		<property name="minLineLength" type="int" value="0"/>
		<property name="includedMethodPatterns" type="array" value="
			/__construct(.*)/,
		"/>
	</properties>
</rule>
<rule ref="SlevomatCodingStandard.Classes.RequireSignleLineMethodSignature">
	<properties>
		<property name="minLineLength" type="int" value="0"/>
		<property name="excludedMethodPatterns" type="array" value="
			/__construct(.*)/,
		"/>
	</properties>
</rule>

There is no BC break. If you don't set excludedMethodPatterns and includedMethodPatterns it will works as before.

@RiKap RiKap requested a review from kukulich January 23, 2020 18:47
Copy link
Contributor

@kukulich kukulich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nice addition.

I've requested some changes. All requests are valid for more commits :)

Please use rebase and fix errors in the right commits.

@@ -392,6 +392,8 @@ Sniff provides the following settings:

* `includedMethodPatterns`: allows to configure which methods are included in sniff detection. This is an array of regular expressions (PCRE) with delimiters.

* `excludedMethodPatterns`: allows to configure which methods are excluded from sniff detection. This is an array of regular expressions (PCRE) with delimiters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document how it works when you set both options together.

Copy link
Contributor

@kukulich kukulich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please remove the commit with readme and change readme in previous four commits when you add relevant option.

  2. Please update all patterns in tests. /__construct(.*)/ does not make sense now.

@RiKap
Copy link
Contributor Author

RiKap commented Jan 28, 2020

@kukulich
2. Why it does not make sense now, but it did before?

@kukulich
Copy link
Contributor

Now you check only method names.

@RiKap RiKap requested a review from kukulich January 29, 2020 11:06
@kukulich kukulich merged commit d1663e1 into slevomat:master Jan 29, 2020
@kukulich
Copy link
Contributor

Thanks.

@RiKap RiKap deleted the patch-1 branch January 29, 2020 15:32
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.

None yet

2 participants