Skip to content

Conversation

@ThomasLandauer
Copy link
Member

No description provided.

@TavoNiievez
Copy link
Member

In the past I was against adding : void at the end of methods because it doesn't add anything relevant to the tests and because it can make the suite tests harder to read.
Strict types were also suggested by me, but it was also not decided to add them.
I even suggested that Cest should be 'final' classes, but again, the simpler the default generated code the better I guess was the reasoning.

@ThomasLandauer
Copy link
Member Author

ThomasLandauer commented Feb 18, 2024

Well, you can certainly see it as noise. But that's the way that PHP has been taking over the last few years.

  • void: psalm is reporting:

    MissingReturnType: Method App\Tests\Acceptance\FirstCest::_before does not have a return type, expecting void

  • declare(strict_types=1); is good practice today
  • And now I even added final - overlooked that yesterday :-)
  • Thinking some more about it, I now even added $scenario (see https://codeception.com/docs/AdvancedUsage) - probably rarely used, but very hard to find in the docs...

My main argument is: That's the way good PHP code should look nowadays, so we should gently guide people to this direction. And the generated files are just a suggestion - anybody can easily delete the stuff they don't want.

@Naktibalda Naktibalda changed the title Update Cest.php: Adding declare(strict_types=1); and return type void to generated files generate:cest: Adding declare(strict_types=1); and return type void to generated files Feb 19, 2024
@TavoNiievez TavoNiievez changed the base branch from 5.1 to 5.2 July 10, 2024 00:19
Copy link
Member

@TavoNiievez TavoNiievez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ThomasLandauer,

In the interest of going forward with this PR I have decided to approve the following changes you argued and I agree with for the next minor version 5.2:

  • declare(strict_types=1);
  • final
  • : void
  • ✅ short comments for each method.

You are of course free to open another PR with the remaining changes:

  • #[Before]. I don't agree with adding this because I think it is adding too much documentation on related methods. Also, the documentation referenced in that link has errors such as "#[Bbefore]".
  • Scenario \$scenario. The use of scenario doesn't seem very common, many people won't need it when writing their tests, so putting it there just to let people know it exists doesn't really seem necessary, since it's unlikely to increase its use.

@ThomasLandauer
Copy link
Member Author

OK, this is a step forward.

For the remaining two, I'm undecided myself...

And see Codeception/codeception.github.com#868 for the typos ;-)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants