spryker / code-sniffer

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

Enable Spryker.Commenting.DocBlockApiAnnotation for project level code #367

Open alfredbez opened 1 year ago

alfredbez commented 1 year ago

We're looking for a sniffer that reports missing Specifications in DocBlocks for the interfaces that need one.

Is there any reason why Spryker.Commenting.DocBlockApiAnnotation is currently limited to code only from the Spryker namespace?

I'm also not sure if this block of code can even work as expected. I think we need to remove the Interface suffix inside \Spryker\Sniffs\AbstractSniffs\AbstractApiClassDetectionSprykerSniff::sprykerApiClass, e.g. like so:

$name = preg_replace('/Interface$/', '', $name);

I can send a PR for that if you like

dereuromark commented 1 year ago

Historically, only facades (and their interfaces) needed and need that specification part. It is quite specific to those type of classes. And if there is an interface (which there always should be), then having it redundant on the specific class would be non-DRY. So from that perspective the focus was on ensuring it to be present on that interface only, and the concrete class method to just use inheritDoc.

alfredbez commented 1 year ago

I'm aware of that, but anyway thanks for the clarification 👍

I just realized that it actually matches also Interfaces, didn't check the whole regex inside the isFacade method.

But the other question is still open:

Is there any reason why Spryker.Commenting.DocBlockApiAnnotation is currently limited to code only from the Spryker namespace?

Would it be possible to remove the prefix in the regex?

@@ -171,7 +171,7 @@
      */
     protected function isFacade(string $namespace, string $name): bool
     {
-        if (preg_match('/^Spryker[a-zA-Z]*\\\\Zed\\\\[a-zA-Z]+\\\\Business$/', $namespace) && preg_match('/^(.*?)(Facade|FacadeInterface)$/', $name)) {
+        if (preg_match('/^[a-zA-Z]*\\\\Zed\\\\[a-zA-Z]+\\\\Business$/', $namespace) && preg_match('/^(.*?)(Facade|FacadeInterface)$/', $name)) {
             return true;
         }
dereuromark commented 1 year ago

Well, the sniff was always only designed to make the core code validated. There is of course nothing wrong to also enforce it on project level as opt-in. If you want to make a PR, I would suggest you allow a config of namespaces (similar to other sniffs) to whitelist, and this way make it BC. Projects shouldnt have failed CI because of this, so best to allow projects to set additional namespaces on top. See https://github.com/spryker/code-sniffer/blob/master/docs/README.md#configure-custom-namespaces If not set it would just default to the current one probably.