Skip to content

Conversation

@JakeQZ
Copy link
Collaborator

@JakeQZ JakeQZ commented Dec 23, 2025

Also include a demo of its usage.

In most classes, a stub that throws an exception is 'implemented'.

Part of #1440.

Also include a demo of its usage.

In most classes, a stub that throws an exception is 'implemented'.

Part of #1440.
@JakeQZ JakeQZ requested a review from oliverklee December 23, 2025 01:25
@JakeQZ JakeQZ self-assigned this Dec 23, 2025
@JakeQZ JakeQZ added enhancement testing PRs/issues adding additional tests only, or primarily testing-focused labels Dec 23, 2025
@JakeQZ
Copy link
Collaborator Author

JakeQZ commented Dec 23, 2025

Notes:

@oliverklee
Copy link
Collaborator

I like this a lot!

A few thoughts:

  • I'd prefer it if we kept the demo implementation out of the first PR.
  • Let's cover the methods with tests! (I think that checking for the exception class should be sufficient, without also checking the exception message.)
  • To keeps things simple for the reader and to allows us to implement the methods one-by-one, I'd prefer it if we only implemented the method in the leaves of the inheritance tree, but not on abstract classes, and if we didn't do parent::getArrayRepresentation calls.
  • Maybe we can add a trait ShortClassRepresentation to avoid having to reimplement the reflection class stuff everywhere. Or we write the class name as a literal (which would further simplify things, and which I would suggest).

WDYT?

@coveralls
Copy link

Coverage Status

coverage: 68.284% (-0.9%) from 69.191%
when pulling 86a006c on test/add-hepler-method
into 0ae1fde on main.

@oliverklee
Copy link
Collaborator

And maybe we can add a corresponding PHPStan custom type to the interface.

@JakeQZ
Copy link
Collaborator Author

JakeQZ commented Dec 24, 2025

  • I'd prefer it if we kept the demo implementation out of the first PR.

Agree. I only included it as a proof-of-concept to see how it would work in practice.

  • Let's cover the methods with tests! (I think that checking for the exception class should be sufficient, without also checking the exception message.)

If I understand correctly, you're suggesting that each class implementing getArrayRepresentation initially has a test method getArrayRepresentationThrows (or suchlike). Then, when we implement the functionality for a specific class, we replace that with a test method that instead confirms it returns the array data expected (for some simple example). Makes sense. Let me know if we're on the same wavelength.

  • To keeps things simple for the reader and to allows us to implement the methods one-by-one, I'd prefer it if we only implemented the method in the leaves of the inheritance tree, but not on abstract classes, and if we didn't do parent::getArrayRepresentation calls.

I think I disagree here :)

Many of the abstract classes contain some properties which should be put in the returned array. It should be the responsibility of those classes to do that for their own properties - not have every 'leaf' class repeating the same job.

I concur that this means that when the method is implemented in a 'leaf' class, it will mean that it also implemented (possibly incompletely) for all of its cousins. I think this is workable - the demo shows that it is.

  • Maybe we can add a trait ShortClassRepresentation to avoid having to reimplement the reflection class stuff everywhere. Or we write the class name as a literal (which would further simplify things, and which I would suggest).

I also thought of adding a trait for that (but didn't go that far). I'd prefer that to writing the class name as a literal, which seems a bit clunky. Some day we may rename some of the classes; having a string literal to update would be an extra change that may be missed.

And maybe we can add a corresponding PHPStan custom type to the interface.

I'm not seeing how this would help. It seems to be about defining an array type with specific keys. We will have different keys depending on the class of object being represented. Neither does it offer a solution to the recursive type problem.

Look forward to your further thoughts, and 🎄🦌☃️🌟🍷 ... have a good Christmas :)

@oliverklee
Copy link
Collaborator

If I understand correctly, you're suggesting that each class implementing getArrayRepresentation initially has a test method getArrayRepresentationThrows (or suchlike). Then, when we implement the functionality for a specific class, we replace that with a test method that instead confirms it returns the array data expected (for some simple example). Makes sense. Let me know if we're on the same wavelength.

Yes, exactly. :-)

As for the naming, maybe we can use getArrayRepresentationThrowsException.

@oliverklee
Copy link
Collaborator

I think I disagree here :)

Many of the abstract classes contain some properties which should be put in the returned array. It should be the responsibility of those classes to do that for their own properties - not have every 'leaf' class repeating the same job.

Okay, I can live with having the method in abstract classes as well. :-)

Merry Christmas to you, too! 🎄

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

Labels

enhancement testing PRs/issues adding additional tests only, or primarily testing-focused

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants