Re: [PATCH v1] SRFI-19: Add support for ISO 8601 zones with a colon.

[Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>]

[-- Attachment #1: Type: text/plain, Size: 2665 bytes --]

Hi,

Sorry for the delay,

On Wed, 06 Mar 2024 12:42:10 -0600
Rob Browning <rlb@defaultvalue.org> wrote:

> Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> writes:
> (...and (not related), I also wondered about making some of the error
>  messsages more specific, i.e. "Invalid time zone minutes digit" or
>  something.)

Would something like that be OK instead?:
> (time-error
>  'string->date 'bad-date-template-string 
>  (list "Invalid time zone number" ch))

My code had indeed big duplication of code: it duplicated the read,
the test and even the error message:
> > +              (if (char=? ch #\:)
> > +                  (set! ch (read-char port))
> > +                  (if (eof-object? ch)
> > +                      (time-error 'string->date
> > 'bad-date-template-string
> > +                                  (list "Invalid time zone number"
> > ch)))) (set! offset (+ offset (* (char->int ch)
> >                                          10 60))))  

And if I understood right the goal of this example is to avoid this
duplication of code:
> This looks reasonable to me -- I wondered about moving the check "up
> front", eliminating the need for the extra eof?, i.e.
> 
>             (let ((ch (read-char port)))
>               (when (char=? ch #\:)
>                   (set! ch (read-char port))
>               (if (eof-object? ch)
>                   (time-error 'string->date 'bad-date-template-string
>                               (list "Invalid time zone number" ch)))
>               (set! ...))
However here (char=? ch #\:) can fail if ch is an eof-object. I tested
that by adding that in some test.scm file:
> (define ch (read-char))
> (char=? ch #\:)
and running guile test.scm and typing ctrl+d to make ch an eof-object,
and that produces the following error:
> In procedure char=?: Wrong type argument in position 1 (expecting
> character): #<eof>

And since we can read a character twice we need to do that eof-object?
test twice.

So would something like that work instead?:
> (let ((f (lambda ()
> 	   (let ((ch (read-char port)))
> 	     (if (eof-object? ch)
> 		 (time-error
> 		  'string->date 'bad-date-template-string
> 		  (list "Missing required time zone minutes" ch))
> 		 ch)))))
>   (let ((ch (f)))
>     (if (char=? ch #\:) (set! ch (f)))
>     (set! offset (+ offset (* (char->int ch) 10 60)))))

Here the downside is that the code is slightly more complex but there
is no duplication of code and it also does one check for each read.
Also note that I didn't test the code above yet.

If all that is good, I can send a v2 of the patch.

Denis.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]