[TASK] Use getArrayRepresentation() in ValueTest

[JakeQZ]

... in parsesArithmeticInFunctions() test.

Follows on from #1440.

[JakeQZ]

Notes:

• PHPStan does not support recursve types so we have to use mixed at the second level;
• This is primarily a WIP to see if our solution for Add helper debugging/rendering function stub for (unit) tests #1440 is viable.

[oliverklee]

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]

coverage: 70.455%. remained the same
when pulling 3947ca3 on test/add-hepler-method
into 1b5be34 on main.

[oliverklee]

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

[JakeQZ]

• 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]

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]

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! 🎄

[oliverklee]

As this is the POC (as I understand it), should I create the non-POC version from it, or are you planning to do that?

[JakeQZ]

You're welcome to. I'm away for a couple more days without access to GitHub (can't sign in from phone due to 2FA so hope this message gets through).

[JakeQZ]

You're welcome to. I'm away for a couple more days without access to GitHub (can't sign in from phone due to 2FA so hope this message gets through).

Am back now. I think we've agreed the way forward. One of us should create the next PR for actual merging, whether based on this or a fresh one. But we should avoid duplication of effort... ;)

[oliverklee]

One of us should create the next PR for actual merging, whether based on this or a fresh one. But we should avoid duplication of effort... ;)

I'm currently still focusing on my end-of year "thinking about my life" ritual and won't work on this until including January 1st. If you want to work on this during this time, please go ahead. (I'll be able to do the reviews, though.)

[JakeQZ]

I'm currently still focusing on my end-of year "thinking about my life" ritual

I do most of that throughout the year whilst trying to get back to sleep in the middle of the night :/

If you want to work on this during this time, please go ahead. (I'll be able to do the reviews, though.)

I've set the ball rolling with #1447. Will keep this PR open for visibility, even though it may never itself be merged.

[JakeQZ]

This now just includes using getArrayRepresentation() in one test method, since other changes have been covered by other PRs, and can be reviewed and merged.

[JakeQZ]

Using assertSame() means the array keys must be in matching order, though it seems slightly illogical for name to be last.

[oliverklee]

Maybe we can use assertEqualsCanonicalizing instead so we can have the array elements in a more readable order in the test.

[oliverklee]

Ah, I take taht back. We'd want something like assertSameCanonicalizing (as we want to do a strict comparison), which does not exist.

[JakeQZ]

Ah, I take taht back. We'd want something like assertSameCanonicalizing (as we want to do a strict comparison), which does not exist.

That would be nice to have. Also, assertEqualsCanonicalizing sorts the array by value, whereas we want it sorted by key to be canonical. I've thought of a way to get CSSFunction to return the array in a more logical order without significant overhead or duplication, which I'll submit as a separate PR if it works...

[JakeQZ]

That said, this particular test is somewhat convoluted. Other simpler tests may not need to worry about the order of the array keys.

[JakeQZ]

I've thought of a way to get CSSFunction to return the array in a more logical order without significant overhead or duplication, which I'll submit as a separate PR if it works...

#1461

[oliverklee]

I've marked the sniffer job as required. So we need to rebase to run all now-required jobs.

[JakeQZ]

I've marked the sniffer job as required. So we need to rebase to run all now-required jobs.

Done.