sqlparser changes to support timestamptz#21
Conversation
|
|
quodlibetor
left a comment
quodlibetor
left a comment
There was a problem hiding this comment.
Sorry to comment some nits on a wip, I was reading for understanding and threw them in.
ruchirK
force-pushed
the
timestamptz-2
branch
from
11b18b5 to
11bdc3a
Compare
ruchirK
changed the title
ruchirK
force-pushed
the
timestamptz-2
branch
3 times, most recently
from
7087fe2 to
f54cc7d
Compare
quodlibetor
left a comment
quodlibetor
left a comment
There was a problem hiding this comment.
Two nit comments, and one request for clarification either via a comment or a refactoring.
| (_, _) => { | ||
| is_positive = true; | ||
| hour_offset = None; | ||
| minute_offset = None; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Even though I recognize that this is obviously working, I don't think that I follow it. Does it unconditionally go through every format string, even if the first one matches? That would explain why this needs to be at the end instead of the beginning. Would it be easier to reason about this if this inner match statement was in an inner function that can signal success by returning a success/fail value?
The reason this feels worth doing is that I believe that the entire function currently depends on the order of items in that format list plus the order of items in this match, and is therefore possibly pretty fragile in the face of refactorings. The tests will prevent people from introducing incorrect code, but I could see trying to interact with this code requiring a lot of understanding.
If you don't feel like experimenting with a refactoring, could you add a comment at the top of the possible formats loop (line 217) explaining the ways that these parts all fit together?
There was a problem hiding this comment.
So the intention was to match on the first format that validly parses (the structure is a doubly nested for loop so the break statement is there when we've detected a mismatch between the format and the actual list of tokens and lets the control flow go to the next one), with the understanding that all of the potential formats with numbers are going to have numbers in HH MM order, so if the first number is >= 24, then we can just fail everything because we know it will never pass any format (as opposed to a token mismatch which could pass a different format)
I'll refactor a bit and add comments
looks great!
ruchirK
force-pushed
the
timestamptz-2
branch
from
390c347 to
805ca84
Compare
3 participants
No description provided.