[TASK] Add Selector\Combinator class

[JakeQZ]

No description provided.

[coveralls]

coverage: 71.792% (+0.4%) from 71.358%
when pulling 68da091 on task/selector-combinator
into 0066316 on main.

[JakeQZ]

Note that this includes the renaming from #1486, which should be reviewed and merged first. Hopefully there won't be subsequent merge conflicts, since the changes overlap,

[oliverklee]

Maybe we can define a custom PHPDoc type for this in the class doc: https://phpstan.org/developing-extensions/custom-phpdoc-types

[JakeQZ]

This looks quite involved. Could this perhaps be done post-PR if considered worthwhile?

[oliverklee]

I'll do that in a minute and push. Hold on …

[oliverklee]

Ah, and the link was incorrect. Now I understand the "quite involved" thing. :-)

I was thinking of a local type alias instead: https://phpstan.org/writing-php-code/phpdoc-types#local-type-aliases

[oliverklee]

Done and repushed.

[oliverklee]

Also, I'd consider it more to be the responsibility of the caller to first trim the value. This would keep our API more clean. WDYT?

[JakeQZ]

I'm fine with that and have removed the trimming. I've added a description to the @throws annotation to indicate what is allowed, since, because this is implementing an interface, it's not possible to tighten the parameter definition.

[oliverklee]

Let's add the name of the tested method:

Suggested change

	public function setsValueProvided(string $value): void
	public function setValueSetsValueProvided(string $value): void

[JakeQZ]

This seems redundant to me. Also the parsing tests were written as parsesXyz(), not parseParsesXyz().

[oliverklee]

This is what I teach developers about naming tests at my testing workshops:
https://speakerdeck.com/oliverklee/test-driven-development-with-phpunit-915a5f2c-3d37-4e41-b5bc-ef1efca9c01e?slide=39

In general, if a test is a bout a specific method, it should include (and preferable start with) the name of the tested method. When there are redundancies in the test name, this usually is a sign of the tested method having a descriptive name, i.e., it should be the case quite often.

And it the test is about the behavior of the class in general, the test name should either start with class or theClass, or this can be implicit: (theClass)implementsCaffeinated.

There are several reasons why I find this important:

1. Having meaningful, complete test names allows the reader to roughly understand what the test is about even without having to read the code of the test method.
2. This allows the reader to compare what the test method name states with the code of the test method, which kind of creates a "double bookkeeping" system which allows us to identify tests testing the wrong thing.
3. Tests not only check for the behavior being correct, but also they serve as documentation of the behavior. So they should be meaningful particularly if the reader is not (yet) familiar with the structure of our code and with what method does what.

Concerning parsing: Yes, we might clean those tests up, and we might want to decide if the parsing is something that "the class" does or the parse method, and rename the tests accordingly.

I'd say it's the latter. WDYT?

[JakeQZ]

OK. I think I understand the reasoning. setsValueProvided() isn't testing a behaviour; it's testing the setValue() method. Whereas parsesXyz() is testing the parsing behaviour, and happens to be also testing the parse() method. So in the latter case I think I still prefer parsesXyz() to parseParsesXyz(). Another one is constructsWithValueProvided() vs constructorConstructsWithValueProvided(). This is testing the instantiation behviour while also testing the constructor, so I think there's an arguable case for either.

I'll change the test method names where they are specifically testing a class method rather than a behaviour...

[oliverklee]

Suggested change

	public function rendersValueProvided(string $value): void
	public function renderRendersValueProvided(string $value): void

[JakeQZ]

It should be clear that we're testing the 'rendering' whatever that actual method that does it is named.

[oliverklee]

See above about redundant naming.

[JakeQZ]

It's slightly arguable that this is testing a behaviour, but I've changed this.

[JakeQZ]

I've changed setValue() to avoid trimming, but am not sure about some of the other suggestions - see comments.

[JakeQZ]

I've added an additional test to confirm that getValue() returns the value passed to setValue() (as well as that passed to the contructor). These are using the tested-method-name-included-in-test-method-name nomenclature. It's how they ended up. I am not sure either way, but do like succincness.