Add standard representation for CharSequence
#3617
Conversation
This also makes the assertion message more helpful when comparing a String and a different CharSequence object.
| protected String toStringOf(String s) { | ||
| return toStringOf((CharSequence) s); | ||
| } | ||
|
|
||
| protected String toStringOf(CharSequence s) { | ||
| return concat("\"", s, "\""); | ||
| } |
There was a problem hiding this comment.
I wasn't sure whether the existing toStringOf(String) should be replaced (since String implements CharSequence), or whether that would be a breaking API change.
There was a problem hiding this comment.
It's not a binary breaking change but if some users rely on that representation then it would be, so I'm inclined we don't change it. Reusing the String representation for char sequence is ok as if the contents are the same we would add the type to differentiate the values.
There was a problem hiding this comment.
Ah sorry my comment was a bit misleading. What I meant is:
- Should there be
toStringOf(String)andtoStringOf(CharSequence)(as done here currently) - Or, should there only be
toStringOf(CharSequence), andtoStringOf(String)is removed
The code would then simply use theCharSequencevariant forStringas well. But removing thetoStringOf(String)overload would technically be an incompatible API change since it isprotectedand users might have overridden it.
There was a problem hiding this comment.
I think it would be better to extract concat("\"", s, "\""); to a private method and call it from both toStringOf, users can then override toStringOf(CharSequence s) without impacting toStringOf(String s).
There was a problem hiding this comment.
I hope the latest changes in 6952df8 are what you had in mind.
There was a problem hiding this comment.
thanks, I'll review that next week, I'm off for a few days
| } | ||
|
|
||
| @Test | ||
| void should_display_class_for_ambiguous_CharSequence() { |
There was a problem hiding this comment.
Not completely sure where the 'right' place for this test is. All these test classes seem to perform variants of ambiguous toString checks:
ShouldBeEqual_Test(this class here)ShouldBeEqual_newAssertionError_differentiating_expected_and_actual_TestStandardRepresentation_unambiguousToStringOf_Test
There was a problem hiding this comment.
That would be in StandardRepresentation_unambiguousToStringOf_Test where we should have exhaustive tests for representation, the other tests are just to check that ShouldBeEqual honors that too but we only one test proving that.
There was a problem hiding this comment.
I think I will completely remove this test method again. StandardRepresentation_unambiguousToStringOf_Test already has one generic test, isEqualTo_should_show_disambiguated_.... Would probably not fit to add this specific String/CharSequence test there.
assertj-core/src/test/java/org/assertj/core/error/ShouldBeEqual_Test.java
Show resolved
Hide resolved
| if (object instanceof String) return toStringOf((String) object); | ||
| if (object instanceof CharSequence) return toStringOf((CharSequence) object); | ||
| if (object instanceof Character) return toStringOf((Character) object); |
There was a problem hiding this comment.
I think technically all these instanceof checks are redundant because they are equivalent to what the parent class StandardRepresentation would do in its toStringOf method (and then call the methods overridden within this class here), but for consistency I added the check for CharSequence here as well.
|
Integrated thanks @Marcono1234 ! |
This also makes the assertion message more helpful when comparing a String and a different CharSequence object. Fix assertj#3617
Currently
CharSequencehas no special handling inStandardRepresentation, so when you accidentally compare aStringwith for example aStringBuilderyou get something like:This assumes that for custom
CharSequencetypes users have overriddentoString, as required by theCharSequenceJavadoc. Otherwise it might be a bit confusing that the result is now enclosed in".Check List:
Following the contributing guidelines will make it easier for us to review and accept your PR.