Skip to content

Conversation

@pscheit
Copy link
Contributor

@pscheit pscheit commented Jun 25, 2025

interestingly, the code is totally fine:

https://github.com/hamcrest/JavaHamcrest/blob/master/hamcrest/src/main/java/org/hamcrest/StringDescription.java#L35

This calls toString, but we actually implemented __toString on the class. So casting as (string) must be fine.

Anyway, the test might be helpful

@pscheit pscheit marked this pull request as draft June 25, 2025 14:59
@pscheit pscheit marked this pull request as ready for review June 25, 2025 15:10
@aik099 aik099 requested a review from Copilot June 25, 2025 15:27
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a missing test to verify that StringDescription::toString properly appends self-describing objects.

  • Added test method testToStringAppendsSelfDescribing in the StringDescriptionTest file
  • Validates that casting the description object to string yields the expected output
Comments suppressed due to low confidence (1)

tests/Hamcrest/StringDescriptionTest.php:168

  • Consider adding additional test cases to cover scenarios with multiple self describing objects or edge cases to further validate comprehensive behavior.
        $description = $this->_description->toString(new \Hamcrest\SampleSelfDescriber('foo'));

@aik099
Copy link
Member

aik099 commented Jun 25, 2025

@pscheit , is this still needed after your comment in the #88?

@pscheit
Copy link
Contributor Author

pscheit commented Jun 25, 2025

It doesnt show what I thought was a bug, no.

But it tests a method that didnt had a test before?

edit: but now that I read your comment, let me extract the fix with the static into this

@aik099
Copy link
Member

aik099 commented Jun 26, 2025

But it tests a method that didnt had a test before?

The public method was tested indirectly (which I don't like personally). So 👍 for the test. Probably we need to double check (as a separate PR) if all the tests actually test connected class at 100% by themselves.

edit: but now that I read your comment, let me extract the fix with the static into this

Without PhpStan I have no way to see if this worked, so I'll merge this anyway.

@aik099 aik099 merged commit 9c8847b into hamcrest:master Jun 26, 2025
6 checks passed
@aik099
Copy link
Member

aik099 commented Jun 26, 2025

Merging, thank you @pscheit .

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