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

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

[Denis 'GNUtoo' Carikli]

* module/srfi/srfi-19.scm (zone-reader): handle a colon in the zone.

* test-suite/tests/srfi-19.test (SRFI date/time library test): Add test.

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
---
 module/srfi/srfi-19.scm       | 5 +++++
 test-suite/tests/srfi-19.test | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/module/srfi/srfi-19.scm b/module/srfi/srfi-19.scm
index 570f933ca..23d115926 100644
--- a/module/srfi/srfi-19.scm
+++ b/module/srfi/srfi-19.scm
@@ -1271,6 +1271,11 @@
               (if (eof-object? ch)
                   (time-error 'string->date 'bad-date-template-string
                               (list "Invalid time zone number" ch)))
+              (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))))
             (let ((ch (read-char port)))
diff --git a/test-suite/tests/srfi-19.test b/test-suite/tests/srfi-19.test
index 1d56214e4..55eb82320 100644
--- a/test-suite/tests/srfi-19.test
+++ b/test-suite/tests/srfi-19.test
@@ -120,6 +120,9 @@ incomplete numerical tower implementation.)"
   (pass-if "string->date works"
 	   (begin (string->date "2001-06-01@14:00" "~Y-~m-~d@~H:~M")
 		  #t))
+  (pass-if "string->date accepts ISO 8601 zones with a colon"
+	   (begin (string->date "2024-12-31T23:59:59+01:00" "~Y-~m-~dT~H:~M:~S~z")
+		  #t))
   ;; check for code paths where reals were passed to quotient, which
   ;; doesn't work in Guile (and is unspecified in R5RS)
   (test-time->date time-utc->date date->time-utc)

base-commit: 9e0f03c5fd36764827c8bb03887f14640c883b70
-- 
2.41.0

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

[Rob Browning]

Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> writes:

> diff --git a/module/srfi/srfi-19.scm b/module/srfi/srfi-19.scm
> index 570f933ca..23d115926 100644
> --- a/module/srfi/srfi-19.scm
> +++ b/module/srfi/srfi-19.scm
> @@ -1271,6 +1271,11 @@
>                (if (eof-object? ch)
>                    (time-error 'string->date 'bad-date-template-string
>                                (list "Invalid time zone number" ch)))
> +              (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))))

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! ...))

(...and (not related), I also wondered about making some of the error
 messsages more specific, i.e. "Invalid time zone minutes digit" or
 something.)

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

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

[Rob Browning]

Oh, and for anyone else reviewing this, some specification-related info:

  https://en.wikipedia.org/wiki/ISO_8601#Time_zone_designators

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

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

[Denis 'GNUtoo' Carikli]

[-- 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 --]

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

[Denis 'GNUtoo' Carikli]

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

On Mon, 11 Mar 2024 19:25:08 +0100
Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> wrote:
> Also note that I didn't test the code above yet.
I tested the code afterward and it passes all the tests.

I'll send a v2 in case that makes review easier.

Denis.

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

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

[Denis 'GNUtoo' Carikli]

* module/srfi/srfi-19.scm (zone-reader): handle a colon in the zone.

* test-suite/tests/srfi-19.test (SRFI date/time library test): Add test.

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
---
ChangeLog v1->v2:
- Improved error message to be more meaningful.
- Removed duplicated code: in the v1, the read, oef-object test, and
  error messages were duplicated.
---
 module/srfi/srfi-19.scm       | 16 ++++++++++------
 test-suite/tests/srfi-19.test |  3 +++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/module/srfi/srfi-19.scm b/module/srfi/srfi-19.scm
index 570f933ca..ad1867506 100644
--- a/module/srfi/srfi-19.scm
+++ b/module/srfi/srfi-19.scm
@@ -1267,12 +1267,16 @@
                               (list "Invalid time zone number" ch)))
               (set! offset (+ offset (* (char->int ch)
                                         60 60))))
-            (let ((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))))
+            (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)))))
             (let ((ch (read-char port)))
               (if (eof-object? ch)
                   (time-error 'string->date 'bad-date-template-string
diff --git a/test-suite/tests/srfi-19.test b/test-suite/tests/srfi-19.test
index 1d56214e4..55eb82320 100644
--- a/test-suite/tests/srfi-19.test
+++ b/test-suite/tests/srfi-19.test
@@ -120,6 +120,9 @@ incomplete numerical tower implementation.)"
   (pass-if "string->date works"
 	   (begin (string->date "2001-06-01@14:00" "~Y-~m-~d@~H:~M")
 		  #t))
+  (pass-if "string->date accepts ISO 8601 zones with a colon"
+	   (begin (string->date "2024-12-31T23:59:59+01:00" "~Y-~m-~dT~H:~M:~S~z")
+		  #t))
   ;; check for code paths where reals were passed to quotient, which
   ;; doesn't work in Guile (and is unspecified in R5RS)
   (test-time->date time-utc->date date->time-utc)

base-commit: 54c4753dd3f7506bee2778b36d7263b613ffd579
-- 
2.41.0

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

[Rob Browning]

Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> writes:

> Would something like that be OK instead?:

Not sure I understand the question.  Were you asking if that'd be OK
elsewhere in the function too?

> However here (char=? ch #\:) can fail if ch is an eof-object.

Apologies for the delay, and good point.  I should have used eqv? rather
than char=?, i.e.

             (let ((ch (read-char port)))
               (when (eqv? 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! ...))

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

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

[Rob Browning]

Rob Browning <rlb@defaultvalue.org> writes:

> Apologies for the delay, and good point.  I should have used eqv? rather
> than char=?, i.e.
>
>              (let ((ch (read-char port)))
>                (if (eqv? 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! ...))

Just checking back -- does that adjustment seem plausible to you?  If
so, I may adjust your patch and proceed with it.

Thanks for the help
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

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

[Denis 'GNUtoo' Carikli]

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

Sorry again for the delay, I had personal issues (that are now mostly
fixed) that needed urgent attention and that took all my time.

On Fri, 05 Apr 2024 17:03:21 -0500
Rob Browning <rlb@defaultvalue.org> wrote:
> Rob Browning <rlb@defaultvalue.org> writes:
> 
> > Apologies for the delay, and good point.  I should have used eqv?
> > rather than char=?, i.e.
> >
> >              (let ((ch (read-char port)))
> >                (if (eqv? 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! ...))  
> 
> Just checking back -- does that adjustment seem plausible to you? 
Yes, that makes the code much better and since when ch is eof, '(eqv? ch
#\:)' returns #f, so it should work.

> If so, I may adjust your patch and proceed with it.
Thanks a lot.

Denis.

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

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

[Denis 'GNUtoo' Carikli]

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

On Tue, 19 Mar 2024 19:21:19 -0500
Rob Browning <rlb@defaultvalue.org> wrote:

> Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> writes:
> 
> > Would something like that be OK instead?:
> 
> Not sure I understand the question.  Were you asking if that'd be OK
> elsewhere in the function too?
I was just asking if my new version looked OK, especially because I'm
new to scheme so I might have missed things I don't know about.

Denis.

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

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

[Rob Browning]

Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> writes:

> Yes, that makes the code much better and since when ch is eof, '(eqv? ch
> #\:)' returns #f, so it should work.

OK, pushed your commit to main.

Thanks for the help
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4