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

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

Open
alfredbez opened this issue Mar 1, 2023 · 3 comments
Open

Comments

@alfredbez
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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.

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

No branches or pull requests

2 participants