- Notifications
You must be signed in to change notification settings - Fork401
Fix #4872: Implement java.util.StringJoiner.#4873
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just some comments RE test cases.
test-suite/shared/src/test/scala/org/scalajs/testsuite/javalib/util/StringJoinerTest.scala OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| val single = new StringJoiner(";", "[", "]").setEmptyValue("--").add("single") | ||
| val multiple = new StringJoiner(";", "[", "]").setEmptyValue("--").add("a").add("b").add("c") | ||
| assertJoinerResult("+++") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe add a test case checking that the empty value of the lhs is taken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hum, isn't that what this test precisely does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, I meant something like:
newStringJoiner(",","{","}").setEmptyValue("+++").merge(empty)
Basically testing thatmerge doesn't override the empty value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah yes I see. Added.
Updated. I also added a bunch of tests withblank elements (empty string elements that make the joiner non-empty). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just some minor comments RE tests.
| val single = new StringJoiner(";", "[", "]").setEmptyValue("--").add("single") | ||
| val multiple = new StringJoiner(";", "[", "]").setEmptyValue("--").add("a").add("b").add("c") | ||
| assertJoinerResult("+++") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, I meant something like:
newStringJoiner(",","{","}").setEmptyValue("+++").merge(empty)
Basically testing thatmerge doesn't override the empty value.
| new StringJoiner(", ", "[", "]").add("one").merge(singleBlank).add("two") | ||
| } | ||
| assertJoinerResult("[single]") { | ||
| new StringJoiner(", ", "[", "]").merge(single) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Consider putting different prefix / suffix tosingle to check that they are not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops, I meant to do that all along. Good catch.
* Support java.util.StringJoiner- ported from Scala.jsscala-js/scala-js#4873
* Support java.util.StringJoiner- ported from Scala.jsscala-js/scala-js#4873(cherry picked from commitb1bba76)
* Support java.util.StringJoiner- ported from Scala.jsscala-js/scala-js#4873(cherry picked from commitb1bba76)
No description provided.