fix regex serialization #11844
fix regex serialization #11844
Conversation
|
How about a test to make sure it doesn't regress again? |
|
Yes. Added regex serialization test. |
| !isequal(a.compile_options, b.compile_options) && return false | ||
| !isequal(a.match_options, b.match_options) && return false | ||
| return true | ||
| end |
There was a problem hiding this comment.
== and isequal do the same thing on all of these types, so the second definition is unnecessary.
There was a problem hiding this comment.
Yes, kept is so as to avoid a regression if any of it changes in future. But probably better to remove it.
| function ==(a::Regex, b::Regex) | ||
| (a.pattern != b.pattern) && return false | ||
| (a.compile_options != b.compile_options) && return false | ||
| (a.match_options != b.match_options) && return false |
There was a problem hiding this comment.
Better:
a.pattern == b.pattern && a.compile_options == b.compile_options && a.match_options == b.match_options
There was a problem hiding this comment.
Yes, that's much more concise.
|
Not sure why having |
|
Probably this line. Basically, when parsing a date, a "transition" delimter (from one date part to another) can be a character, string, or regex. This code probably needs a cleanup. |
|
@quinnj Changing that to Any ideas? |
|
|
||
| function ==(a::Regex, b::Regex) | ||
| a.pattern == b.pattern && a.compile_options == b.compile_options && a.match_options == b.match_options | ||
| end |
There was a problem hiding this comment.
@StefanKarpinski do we need to define hash also?
There was a problem hiding this comment.
Sure. This ought to do it:
const hashre_seed = UInt === UInt64 ? 0x67e195eb8555e72d : 0xe32373e4
function hash(r::Regex, h::UInt)
h += hashre_seed
h = hash(r.pattern, h)
h = hash(r.compile_options, h)
h = hash(r.match_options, h)
end
|
Changing that line to |
|
Ok – once this passes CI and gets merged, we can revert that commit and make your change. |
also: - add regex comparison method and serialization test - define hash for regex - fix date parsing
|
Poor travis is getting a beating this week! |
|
Yeah, unfortunately. But we got a green from Travis now! |
fix regex serialization
| @@ -71,7 +71,7 @@ function DateFormat(f::AbstractString,locale::AbstractString="english") | |||
| for (i,k) in enumerate(s) | |||
| k == "" && break | |||
| tran = i >= endof(s) ? r"$" : match(Regex("(?<=$(s[i])).*(?=$(s[i+1]))"),f).match | |||
| slot = tran == "" || tran == r"$" ? FixedWidthSlot : DelimitedSlot | |||
| slot = tran == "" ? FixedWidthSlot : DelimitedSlot | |||
There was a problem hiding this comment.
So why was this change reasonable? Was the logic just wrong before and happened to work because of the lack of an equality operator for regular expressions?
There was a problem hiding this comment.
I think so. If tran <: Regex, slot must be a DelimitedSlot for the correct getslot method to be invoked. It was working till now because the equality test on Regex types was failing.
@quinnj ?
There was a problem hiding this comment.
Yep, I think that's the case. The slot-parsing code here went through a few iterations, so my guess is that it may have been more relevant at an earlier iteration, but then didn't matter as much and didn't cause anything else to fail since it was always false.
This should fix serialization of
Regextypes broken by recent changes.Also added a test, and comparison methods (required for the test).