Hi Joe, here's a partial review:
On Sun, Apr 9, 2023 at 5:44 PM Joseph Koshakow <koshy44@gmail.com> wrote:
> 1) Removes dead code for handling unit type RESERVE.
Looks good. From a quick skim it looks like the ECPG copy of this code
(ecpg/pgtypeslib/interval.c) might need to be updated as well?
> 2) Restrict the unit "ago" to only appear at the end of the
> interval. According to the docs [0], this is the only valid place to
> put it, but we allowed it multiple times at any point in the input.
Also looks reasonable to me. (Same note re: ECPG.)
> 3) Error when the user has multiple consecutive units or a unit without
> an accompanying value. I spent a lot of time trying to come up with
> robust ways to detect this and ultimately settled on using the "type"
> field. I'm not entirely happy with this approach, because it involves
> having to look ahead to the next field in a couple of places. The other
> approach I was considering was to introduce a boolean flag called
> "unhandled_unit". After parsing a unit it would be set to true, after
> applying the unit to a number it would be set to false. If it was true
> right before parsing a unit, then we would error. Please let me know
> if you have any suggestions here.
I'm new to this code, but I agree that the use of `type` and the
lookahead are not particularly obvious/intuitive. At the very least,
they'd need some more explanation in the code. Your boolean flag idea
sounds reasonable, though.
> There is one more problem I noticed, but didn't fix. We allow multiple
> "@" to be sprinkled anywhere in the input, even though the docs [0]
> only allow it to appear at the beginning of the input.
(No particular opinion on this.)
It looks like this patch needs a rebase for the CI, too, but there are
no conflicts.
Thanks!
--Jacob