spryker / code-sniffer

Spryker Code Sniffer
https://spryker.com
MIT License
36 stars 11 forks source link

Add new sniffers for strict_types declaration #356

Closed DmytroKlymanSpryker closed 1 year ago

DmytroKlymanSpryker commented 2 years ago

PR Description

Checklist

dereuromark commented 1 year ago

Can we separate this into allowing/fixing order, and a different PR to "require" it? Since it wouldnt BC to just release this now.

kalaputsik commented 1 year ago

@dereuromark I'm a little bit confused... why we can't change the order of the methods if we've just added them into current PR?

dereuromark commented 1 year ago

What I mean is: CS ruleset should fix the position/order of the "strict_types" element (if it is already existing in the file). It should be after the file docblock as per PSR as I understand.

But it should not auto-add them if missing right now. At least not for projects, this would be BC breaking in many cases. So this needs to be an opt-in sniffer rule for now, or we need to release another sem major soon.

So my question: Can we separate the order fixing from the adding (as requirement)?

DmytroKlymanSpryker commented 1 year ago

What I mean is: CS ruleset should fix the position/order of the "strict_types" element (if it is already existing in the file). It should be after the file docblock as per PSR as I understand.

But it should not auto-add them if missing right now. At least not for projects, this would be BC breaking in many cases. So this needs to be an opt-in sniffer rule for now, or we need to release another sem major soon.

So my question: Can we separate the order fixing from the adding (as requirement)?

So... I completely reworked the sniffer and fixer. At the moment it checks that declare(strict_types=1); should be on the right position in the file if it is already defined somewhere in the file.

If it is not defined anywhere in the file, nothing will be changed and no notifications will be added.

If it exists in the file, but on the wrong position, fixer will place it on the correct position. Example of how it works is available in this PR.

But I introduced the new parameter strictTypesMandatory. By default it is false, but if it will be set on true, sniffer will automatically check that declare(strict_types=1); MUST exist in the file, and fixer will automatically add it on the right place. So it can be activated on the project level, by default it is deactivated.

dereuromark commented 1 year ago

Nice work!

dereuromark commented 1 year ago

Should we add some documentation into readme?

DmytroKlymanSpryker commented 1 year ago

Should we add some documentation into readme?

I wanted to write the documentation for the new sniffer, describe all parameters, but in README we don't have the description for any of our 30+ custom Spryker sniffers. There is just a general information how to write the own sniffer, so I left it without changes.

dereuromark commented 1 year ago

Well, most dont have custom options The ones that do should be documented if thats the key Element to use and activate them

dereuromark commented 1 year ago

I added some docs in https://github.com/spryker/code-sniffer/commit/da643f4f8673d2660a0c7afe1709df2ac53797b0 Also ran the missing composer docs part.

dereuromark commented 1 year ago

Interesting I checked the ruleset, and PSR2 is included, but some of the sniffs only warn, they dont error and fix

So here: https://github.com/spryker/code-sniffer/pull/357