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

[WIP] Exclude fluent interfaces from OneObjectOperatorPerLineSniff #43

Conversation

UFOMelkor
Copy link
Contributor

Allows the OneObjectOperatorPerLineSniff ignore fluent interfaces.

Fluent interfaces are recognized by its starting points. A starting point can be either a variable (like $queryBuilder) or a method call (like createQueryBuilder()).
In both cases the fluent interface might end before the end of the call chain. Therefore end points (like getQuery() will let the sniff work normally for the calls that are detected afterwards.

There are some things to clarify:

  • Should there are any defaults for the starting / end points? IMHO this is user specific, so no defaults would be the better way. (Didn't implemented it yet because it will make the test execution harder.)
  • Configuration of Sniffs is done by public properties, but this violates the object-calisthenics ruleset.

Closes #41

@TomasVotruba
Copy link
Contributor

This looks nice and clear. Thank you for the PR!
What needs to be done here?

I'm about to add these sniffs to my code: https://github.com/Symplify/Symplify
So I should test them hard! :)

@UFOMelkor
Copy link
Contributor Author

I would suggest the following steps:

  • Create another ruleset in tests\rulesets.xml that imports src\ObjectCalisthenics\ruleset.xml but excludes the ObjectCalisthenics.Classes.PropertyVisibility sniff.
  • Adjust the composer.json to use tests\rulesets.xml instead of src\ObjectCalisthenics\ruleset.xml.

Not really cool, but as long as PHP_CodeSniffer offers no other possibility to configure rules, we depend on public properties.

Next step would be thinking about defaults. The only fluent interfaces I use are the doctrine ones. But I'm sure there are a lot more (thinking e.g. of the Symfony Finder component).
Therefore I suggest using no defaults but add a documentation about how to configure it by yourself.

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Feb 28, 2017

That's another issue, but I'm aware of it.

Actually I'm finishing layer above PHP_CodeSniffer that would allow writing rules in clean way. I like it the way PHP-CS-Fixer solves this via configure() method.


Maybe we could approach this via configuration, to skip this rule for classes of certain name (*Sniff) or implementing certain interface/class (Sniff). What do you think?

@UFOMelkor
Copy link
Contributor Author

That sounds good to me. So next steps would be

  • Add an exclude-pattern *Sniff.php to the rule ObjectCalisthenics.Classes.PropertyVisibility
  • Remove default values and adjust the tests
  • Document the configuration (perhaps here?)

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Feb 28, 2017

@UFOMelkor I agree, let's do this. Could you prepare PR for that?

I'm about to simplify the README text, so normal people could read it with joy and understand it.

Thank you so much for your activity. It drives me ahead and motivates to work on this and make it better and better!

@TomasVotruba
Copy link
Contributor

What needs to be done here related to your PR with excludable objects?
I might merge it to current branch manually, if this is ready.

I took care of the public property issue.

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Mar 10, 2017

Merged via 4c0168f, thank you.

Feel free to check and test it.

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Mar 10, 2017

Todo

Could you take care of this?

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

Successfully merging this pull request may close these issues.

None yet

2 participants