Skip to content

Conversation

@kenjis
Copy link
Member

@kenjis kenjis commented Nov 9, 2023

Description
TestResponse is not internal, but devs use it.
But php-cs-fixer force to add it. How can we remove it?

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

TestResponse is not internal, but devs use it.
@kenjis kenjis added the help wanted More help is needed for the proper resolution of an issue or pull request label Nov 9, 2023
@PushoDev

This comment was marked as resolved.

@MGatner
Copy link
Member

MGatner commented Nov 20, 2023

Well, that was odd. But I agree with the approach: add TestResponse to the CS Fixer exemption list for now, and maybe include a note in a file-level comment. This file is unlikely to change and also have style issues, but we may be creating static analysis problems for devs downstream.

@paulbalandan
Copy link
Member

php-cs-fixer is adding the @internal phpdoc because it extends TestCase and thinks it is a test.

@paulbalandan
Copy link
Member

Does TestResponse really need to extend TestCase? I think we can make it a class of its own with no extends. The use for extending TestCase, from what I can see, is to use the assertions. Since those are actually static, we can directly use them instead.

@kenjis
Copy link
Member Author

kenjis commented Nov 22, 2023

TestResponse is not a kind of TestCase.
So it should not extend TestCase.

@MGatner
Copy link
Member

MGatner commented Nov 23, 2023

Agreed! That was my mistake, I believe. Static assertions would be way better.

@paulbalandan paulbalandan removed the help wanted More help is needed for the proper resolution of an issue or pull request label Nov 30, 2023
@paulbalandan
Copy link
Member

Closing in favor of #8264

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.

4 participants