Remove void return type from tests #223
Conversation
| I really feel like this is only a symfony/symfony concern 😞 |
| But the used Fabbot action is now shared across all repos, or shall we make this configurable @nicolas-grekas ? See |
| @nicolas-grekas any clue regarding? |
| This is a general Symfony policy. Adding return types to test cases is just visual debt (TM) |
| ah okay, i thought it's more about avoiding git conflict between different versions - which does not apply here. is it that you opt in whenever there is a more meaningful return type for tests depending on each other or just avoid that in general? not that i see depending test scenarios a lot, but that's the difference to anyhow no hard feelings - just curious :D |
| I would not enforce this rule in a brand new code repository, tbh. I understand that this rule has historic reasons in symfony/symfony, but those don't apply here. Let's use types everywhere. The less exceptions to the rule, the better. |
| I agree ☝🏻 |
There was a problem hiding this comment.
The search-n-replace has been a bit too broad ;)
This still makes sense to me. It's not about historical practices, it's about thinking about what one is doing. Each method needs careful thinking at its boundaries, to provide a clear API.
Test case methods are not part of any API. They're entry points. Specifying a return type for them is just more syntax soup IMHO.
A rule like "all methods must have a return type" applies in a context. Ignoring this context (designing APIs) and making the rule a generic one prevents people from thinking about what they're doing.
I wouldn't call the "no-void" policy an exception to the rule, but more like a domain where the above rule doesn't apply.
| } | ||
| | ||
| private function assertToolConfiguration(Tool $metadata, string $className, string $name, string $description, string $method, array $parameters): void | ||
| private function assertToolConfiguration(Tool $metadata, string $className, string $name, string $description, string $method, array $parameters) |
There was a problem hiding this comment.
| private function assertToolConfiguration(Tool $metadata, string $className, string $name, string $description, string $method, array $parameters) | |
| private function assertToolConfiguration(Tool $metadata, string $className, string $name, string $description, string $method, array $parameters): void |
That's because the --- a/.php-cs-fixer.dist.php +++ b/.php-cs-fixer.dist.php @@ -43,7 +43,7 @@ return (new PhpCsFixer\Config()) ->setRiskyAllowed(true) ->setFinder( (new PhpCsFixer\Finder()) - ->in([__DIR__.'/demo', __DIR__.'/examples', __DIR__.'/fixtures', __DIR__.'/src']) + ->in(__DIR__.'/{demo,examples,fixtures,src}') ->append([__FILE__]) |
| For phpstan to be happy, I'd suggest going with this, which matches what fabbot does (we could further restrict to ignoreErrors: +- +message: "#^Method .*::test.*\\(\\) has no return type specified\\.$#" + |
| Will tackle that topic tomorrow |
| Yea, i guess it's rather subjective how you read these methods, but I get your point - and I also don't want to be bothered to define a complex array shape for a |
| @nicolas-grekas if I get it right, we don't have a convenient link to fix CS for the contributors anymore, this is a step back IMHO |
| You're right, that's not something supported by GHA. |
| See #226 for exception messages |
nicolas-grekas force-pushed the remove-void-return-types-from-tests branch 3 times, most recently from 3ebdd14 to 3f93cca Compare
Lets go with yours first, and then rebase mine on top |
nicolas-grekas force-pushed the remove-void-return-types-from-tests branch 2 times, most recently from 5739c7c to 3f0231e Compare 
nicolas-grekas force-pushed the remove-void-return-types-from-tests branch from 3f0231e to 2f97be9 Compare | Thank you @OskarStark. |
| Thank you @OskarStark. |
4 participants
To make Fabbot (and @nicolas-grekas) happy 😃