Fix #1171: Split: Sometimes remove leading empty match #1198
Conversation
|
Review by @sjrd |
|
Test FAILed. |
|
|
||
| val result = new js.Array[String](0) | ||
| val result = mutable.Buffer.empty[String] |
There was a problem hiding this comment.
Why the change? I would keep new js.Array, here (potentially without length argument, as then it will become simply []).
There was a problem hiding this comment.
Mainly removed it so we can drop the loop that copies to a scala.Array. But I can revert to js.Array if you prefer.
There was a problem hiding this comment.
Well now you can use .toArray on a js.Array. You don't need the loop.
And mutable.Buffer.empty creates a js.Array under the hood anyway, nowadays, so your argument is void.
|
That's all. Quite an "experiment-driven development" ^^ |
| result += inputStr.substring(prevEnd) | ||
|
|
||
| // Remove a leading empty element iff there is no other place the | ||
| // regex matches (than at the beginning) |
There was a problem hiding this comment.
Forgive my lack of tact here but there's no way this is the correct logic. It should be:
- r = input
- if r matches, store left-match in result-array, set r to right-match
- loop until r empty or no match.
- store r in result-array if non-empty.
There was a problem hiding this comment.
This is not the splitting logic, but post-processing (and it actually is wrong, but for another reason). The splitting logic is in the while-loop just above and is almost as you describe (we actually have to discard the matching string, rather than adding it to the result).
There was a problem hiding this comment.
The post-processing logic is wrong, because it should only remove the leading empty element if the match that produced it is zero-length. Consider this:
"abc".split("a") // -> Array("", "bc")There was a problem hiding this comment.
Oops, by left-match I meant stuff left of the current match (since last match) while, yes, I agree the match itself is gobbled.
|
Now that this patch shows me where to make changes and where to add tests, I could build on it if you like? I'm always using regex, I think I could fix & improve it. Different question, do we expect regex in Scala.js output to behave the same as JVM regex? Because if so there are a bunch of another regex things I've noticed don't work... |
The conditions were determined experimentally since the JavaDoc is note precise enough.
|
Updated |
|
@japgolly About your general question about regex: Of course the ideal situation would be that Regex in Scala.js behaves exactly like on the JVM. However, since Scala.js uses JavaScript's regexes to implement the JVM regexes, there are inherent semantic differences. It would probably be exaggerated to package a regex engine written in JavaScript with Scala.js just to have the same semantics (although these things exist). On the other hand, if a semantic alignment with the JVM is easy (in terms of implementation on Scala.js' side), that is definitely desirable (and PRs & tests are very welcome). |
|
Test FAILed. |
|
retest this please |
|
LGTM, even though I fear we're not done yet with this method ^^ |
|
Test FAILed. |
|
retest this please |
|
Test PASSed. |
Fix #1171: Split: Sometimes remove leading empty match
|
@gzm0 Gotcha. It's the dependence on the JS regex that makes things difficult (as I'm sure you know). Hold on, let me move this conversation elsewhere... |
|
Test PASSed. |
The conditions were determined experimentally since the JavaDoc is
note precise enough.