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

Convert Assert<String> extensions into Assert<CharSequence> extensions. #309

Merged
merged 1 commit into from
Aug 14, 2020

Conversation

Kritarie
Copy link
Contributor

Most of the diff is a simple cut-paste from strings.kt and StringTest.kt to charsequence.kt and CharSequence.kt.
Slight modifications were required for isEqualTo and isNotEqualTo, as they both relied on the String? extension of .equals in the standard lib.
Also adds two tests that caught a bug I introduced with this change, specifically when comparing null to "null"

Couple thoughts:

  1. Is there anything I'm missing that might cause a maintenance headache with this change?
  2. Are there cases where this will break compilation for consumers?

@Kritarie Kritarie requested a review from evant August 14, 2020 14:06
Copy link
Collaborator

@evant evant left a comment

Choose a reason for hiding this comment

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

  1. For compilation this is fine, have been only caring about source compatibility and not binary compatibility at this point since we are < 1.0 and similar changes have been made.

fun Assert<CharSequence?>.isEqualTo(other: CharSequence?, ignoreCase: Boolean = false) = given { actual ->
val actualString = actual?.toString()
val otherString = other?.toString()
if (actualString.equals(otherString, ignoreCase)) return
Copy link
Collaborator

Choose a reason for hiding this comment

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

So it's possible for a CharSequence to not be equal but their toString() representations to be equal (Think spanned strings on android). Think we need to keep these only for String.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍

* @param ignoreCase true to compare ignoring case, the default if false.
* @see [isEqualTo]
*/
fun Assert<CharSequence?>.isNotEqualTo(other: CharSequence?, ignoreCase: Boolean = false) = given { actual ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto for isNotEqualTo

@Kritarie Kritarie force-pushed the string-to-charsequence branch from 367c383 to ce77b28 Compare August 14, 2020 16:00
@Kritarie Kritarie requested a review from evant August 14, 2020 16:01
@evant evant merged commit 40f640b into main Aug 14, 2020
@evant evant deleted the string-to-charsequence branch August 14, 2020 17:06
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.

None yet

2 participants