We currently have two @codingStandardsIgnoreFile directives in our code. One for the auto-generated \Drupal\search_api\ProxyClass\ParamConverter\SearchApiConverter class and one for \Drupal\search_api\UnsavedIndexConfiguration.

I think we should remove both of them, but definitely the latter. That one was added in #2671288-51: Follow drupal coding standards, probably because the code was unused at the time so it didn't seem worthwhile to adapt it to coding standards. Since that doesn't apply anymore, we should remove the directive again and fix any resulting code style issues.

We should be able to do the same for SearchApiConverter by just rephrasing or reformatting some of the text.

Comments

drunken monkey created an issue. See original summary.

borisson_’s picture

Since SearchApiConverter is autogenerated, I don't think we should fix it. Core has a couple of similar files as well; see core/modules/node/src/ProxyClass/ParamConverter/NodePreviewConverter.php

drunken monkey’s picture

Since SearchApiConverter is autogenerated, I don't think we should fix it.

Why? I understand not changing auto-generated files if they will be re-generated at some point, or if the generating code also has to read them again, but none of that seems to apply. To me, it seems there's no real argument here against fixing coding standards in that file.
(And especially regarding coding standards, "Core also does it" is no valid argument.)

I'm not hell-bent on doing it, though, I just don't see a reason why not.

borisson_’s picture

I don't see a reason why we should do it. Generated files are not something we should worry about.

borisson_’s picture

Actually, both files are autogenerated. I wanted to start working on this but I feel like we can all use our time better :) Let's keep those 2 files as-is, close this issue and move on.

drunken monkey’s picture

Actually, both files are autogenerated. I wanted to start working on this but I feel like we can all use our time better :) Let's keep those 2 files as-is, close this issue and move on.

Of course we can use our time better, especially as long as there's no stable release. But if that's a reason to close an issue, 99% of issues on d.o could just be closed. What harm is there in keeping it open, and seeing if maybe some "novice" comes along who wants to work on this?

dev.patrick’s picture

Assigned: Unassigned » dev.patrick
Status: Active » Needs review
StatusFileSize
new492 bytes

Adding patch to remove @codingStandardsIgnoreFile at \Drupal\search_api\ProxyClass\ParamConverter\SearchApiConverter

drunken monkey’s picture

StatusFileSize
new1006 bytes
new1.02 KB

Thanks!
However, PHPCS then complained (at least for me locally – the test bot doesn't seem to use that check) that namespaced classes aren't supposed to have a file comment, so I moved that to the class doc comment instead.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new2.6 KB

Applied the patch and ran phpcs, no warnings.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

I don't think the attachment was intended? ;)
Anyways, thanks for reviewing!
Committed. Thanks again, everyone!

borisson_’s picture

No, looks like that was from another issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.