Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TextIs should test asString(), toString(), equals() and hashCode() #199

Open
fabriciofx opened this issue Sep 25, 2020 · 7 comments
Open

Comments

@fabriciofx
Copy link

Actually, TextIs matcher just checks if two Text#asString() are equals. IMHO, to two Text are truly equals, asString(), toString(), equals() and hashCode() must be equals, because:

  • asString() and toString()` are the same thing, should be equals;
  • If both are equals, by logic, equals() must be equals too;
  • As hashCode() is closely linked to equals(), both hashCode() should be equals too.

So, @victornoel and @llorllale WDYT?

@victornoel
Copy link
Collaborator

@fabriciofx yes, I agree.

And also, if someone just want to test that two Text are equal without the extra checks, they can use the IsEqual matcher.

For the record, this ticket comes from a discussion there: yegor256/cactoos#1472.

@llorllale
Copy link
Owner

@fabriciofx I agree with all but toString(): there are no identity semantics attached to its definition.

@victornoel
Copy link
Collaborator

@llorllale I was wondering also about toString(): in cactoos, it is kind of expected that the contract of Text is that toString() returns the same thing as asString(), no?

@llorllale
Copy link
Owner

@victornoel no one should rely on Text.toString() being equal to Text.asString(). Seems to me like if anyone is then they're actually relying on implementation of TextOf which, again, has no commitment to having those two equal.

Are we thinking about formatting strings using Text here? That's the only practical place I see where it would make sense for Text.toString() be equal to Text.asString().

@victornoel
Copy link
Collaborator

@llorllale I see your point. I'm a bit confused about the meaning of toString for Text then... what is it?

@andreoss
Copy link
Contributor

andreoss commented Nov 6, 2020

This issue was also brought up here yegor256/cactoos#1485 (comment)

@fabriciofx
Testing for equals & hashCode breaks TextIs for implementations that are not based upon TextEnvelope + TextOf, in cactoos it's text.Sticky. The contract for equals/hashCode [3] is not obvious for a user who implements Text .
But since TextOf uses original string for hashCode /equals, when TextIs checks for equals it's no different than new IsEqual<>(new TextOf(...))

Btw, TextIs and TextHasValue are almost identical (the later calls contains instead of equals).
And there is no way to write something like new TextHasValue(Matchers.regexp("...."))

Also, should TextIs be named IsText?

1: https://github.com/llorllale/cactoos-matchers/blob/master/src/main/java/org/llorllale/cactoos/matchers/TextHasString.java
2: https://github.com/llorllale/cactoos-matchers/blob/master/src/main/java/org/llorllale/cactoos/matchers/TextIs.java
3: https://github.com/yegor256/cactoos/blob/master/src/main/java/org/cactoos/text/TextOf.java#L641

@victornoel
Copy link
Collaborator

@andreoss just reacting about the point about equals: every implementation of text could/should implement it. You don't have to rely on TextOf for that. It's not ideal and it's not 100% clear if the text interface contract should include equals and hashcode but for now, it seems to be the case...

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

No branches or pull requests

5 participants