Re: New ‘--list-generations’ and ‘--delete-generations’ options
Nikita Karetnikov skribis: >> Cool. The diff looks great... but it lacks tests. > > Yes, I wanted to finish the recutils part first. I think it’s reasonable to have a first milestone without recutils output. > I’ve been thinking about that, and I don’t like that we’d have to use > two record sets. It’d be necessary to post-process the output nearly > every time. Then why bother? We can already filter the output using > regexps. Not sure what you mean by “two record sets”. I see two use cases: one where you just want human-friendly output, for when one is glancing at the available generations, and one that is more amenable to Unix pipelines for post-processing. Recutils output is for the latter case. > I still think that someone may benefit from the recutils format. So > let’s allow the ‘recutils’ argument that would list all generations in > that format [1] and use the following format [2] for everything else: > > generation 1 Dec 16 2013 > guile2.0.7 out,debuggnu/packages/guile.scm > hello2.8 out gnu/packages/base.scm > > generation 2 May 7 2013 > guile2.0.9 out gnu/packages/guile.scm > > Is it OK? Yes. > Should it point to the store instead of (gnu packages …)? Yes, I think so. > Also, why did you propose to use ‘object->fields’? Should I create an > SRFI-9 record representing the recutils fields? ‘object->fields’ is just a convenient way to serialize an object in recutils format. So no, you don’t need a new record type for the recutils field. Instead you would just serialize the objects that represent a profile/manifest using that (currently there’s no distinct record type for those, but the idea is the same.) >> What you could do is add the test cases you already have to >> tests/profile.scm, say (or tests/ui.scm for ‘string->duration’, and then >> put that one in (guix ui)?), along with a simple test in >> tests/guix-package.sh. > > Why do you suggest to put ‘string->duration’ into (guix ui)? Because it’s a user-interface function. > I think we should try to write all tests in Scheme, so we could switch > to property-based testing at some point (see [3], for instance). We still want to test the command-line user interface too. That’s why there’s tests/packages.scm *and* tests/guix-package.sh, for instance. Besides, I’m all for QuickCheck-style tests, there just wasn’t any ready-to-use lib for Guile at the time (I think Ian Price has written/ported one in the meantime, though.) >>> (Is it necessary to mention that ‘maybe-comma-separated-integers’ accepts >>> something like ‘1,2,3,’ or ‘1,,,2’. Or should I change the function?) > >> That’s OK. > > Hmmm, it feels sloppy, so I’ve changed the function: Even better. :-) >>> +(define (string->generations str) >>> + "Return a list of generations matching a pattern in STR. This function > >> Return *the* list of... > > Done. > > I never know which article should be used in such cases; the docstring > talks about it for the first time… On the other hand, it talks about a > particular object. How do you distinguish these cases? Here we’re using Scheme lists to represent a set, and there can be only one set of generations matching the given pattern; that’s why I suggested ‘the’ instead of ‘a’. In general I like to remove any ambiguity, and using ‘a’ is often a source of ambiguity. >>> guix package: error: build failed: getting attributes of path >>> `/nix/store/fcwh19ljibqjfx0c3cwnwcc7p31aq227-glibc-2.17-locales': No such >>> file or directory > >> Arf, what have you done? > > I don’t know! > >> Maybe you can try ‘nix-store --verify’ > > I installed Nix 1.5.3. and ran the command: > > error: setting synchronous mode: unable to open database file Problem with permissions on the SQLite database, I guess. Thanks, Ludo’.
Re: New ‘--list-generations’ and ‘--delete-generations’ options
> Cool. The diff looks great... but it lacks tests. Yes, I wanted to finish the recutils part first. I’ve been thinking about that, and I don’t like that we’d have to use two record sets. It’d be necessary to post-process the output nearly every time. Then why bother? We can already filter the output using regexps. I still think that someone may benefit from the recutils format. So let’s allow the ‘recutils’ argument that would list all generations in that format [1] and use the following format [2] for everything else: generation 1 Dec 16 2013 guile2.0.7 out,debuggnu/packages/guile.scm hello2.8 out gnu/packages/base.scm generation 2 May 7 2013 guile2.0.9 out gnu/packages/guile.scm Is it OK? Should it point to the store instead of (gnu packages …)? Also, why did you propose to use ‘object->fields’? Should I create an SRFI-9 record representing the recutils fields? > What you could do is add the test cases you already have to > tests/profile.scm, say (or tests/ui.scm for ‘string->duration’, and then > put that one in (guix ui)?), along with a simple test in > tests/guix-package.sh. Why do you suggest to put ‘string->duration’ into (guix ui)? I don’t think that it could be reused anywhere. I think we should try to write all tests in Scheme, so we could switch to property-based testing at some point (see [3], for instance). >> (Is it necessary to mention that ‘maybe-comma-separated-integers’ accepts >> something like ‘1,2,3,’ or ‘1,,,2’. Or should I change the function?) > That’s OK. Hmmm, it feels sloppy, so I’ve changed the function: (define (maybe-comma-separated-integers) (let ((lst (delete-duplicates (map string->number (string-split str #\,) (and (every integer? lst) lst))) It shouldn’t be a problem since the code returns an error message: $ ./pre-inst-env guix package -l 1,2, guix package: error: invalid syntax: 1,2, $ ./pre-inst-env guix package -l 1,,,2 guix package: error: invalid syntax: 1,,,2 >> +(define (string->generations str) >> + "Return a list of generations matching a pattern in STR. This function > Return *the* list of... Done. I never know which article should be used in such cases; the docstring talks about it for the first time… On the other hand, it talks about a particular object. How do you distinguish these cases? (I’ve also changed other comments and docstrings.) >> +(define* (available-generations str #:optional (profile %current-profile)) > Perhaps ‘matching-generations’? Done. >> guix package: error: build failed: getting attributes of path >> `/nix/store/fcwh19ljibqjfx0c3cwnwcc7p31aq227-glibc-2.17-locales': No such >> file or directory > Arf, what have you done? I don’t know! > Maybe you can try ‘nix-store --verify’ I installed Nix 1.5.3. and ran the command: error: setting synchronous mode: unable to open database file > (and port that option to Guix while you’re at it ;-)). OK, I added it to my todo list. But I haven’t finished with generations and MIPS yet. [1] https://lists.gnu.org/archive/html/guix-devel/2013-09/msg00097.html [2] https://lists.gnu.org/archive/html/guix-devel/2013-08/msg00126.html [3] https://github.com/ijp/quickcheck/blob/master/quickcheck.sls pgpCSwf35g4tQ.pgp Description: PGP signature
Re: New ‘--list-generations’ and ‘--delete-generations’ options
Nikita Karetnikov skribis: >> I’d prefer clearer case analysis as shown above. > > OK, what do you think about this diff? If everything is fine, I’ll make > it output generations in the recutils format. Cool. The diff looks great... but it lacks tests. What you could do is add the test cases you already have to tests/profile.scm, say (or tests/ui.scm for ‘string->duration’, and then put that one in (guix ui)?), along with a simple test in tests/guix-package.sh. WDYT? > (Is it necessary to mention that ‘maybe-comma-separated-integers’ accepts > something like ‘1,2,3,’ or ‘1,,,2’. Or should I change the function?) That’s OK. > I don’t know if the code works with non-default profiles because my > store is broken. When I try to install or build a new package (with or > without substitutes), I get the following message: > > guix package: error: build failed: getting attributes of path > `/nix/store/fcwh19ljibqjfx0c3cwnwcc7p31aq227-glibc-2.17-locales': No such > file or directory Arf, what have you done? Maybe you can try ‘nix-store --verify’ (and port that option to Guix while you’re at it ;-)). Minor things: > +(define (string->generations str) > + "Return a list of generations matching a pattern in STR. This function Return *the* list of... > +(define* (available-generations str #:optional (profile %current-profile)) Perhaps ‘matching-generations’? Thank you! Ludo’.
Re: New ‘--list-generations’ and ‘--delete-generations’ options
> I’d prefer clearer case analysis as shown above.
OK, what do you think about this diff? If everything is fine, I’ll make
it output generations in the recutils format.
(Is it necessary to mention that ‘maybe-comma-separated-integers’ accepts
something like ‘1,2,3,’ or ‘1,,,2’. Or should I change the function?)
I don’t know if the code works with non-default profiles because my
store is broken. When I try to install or build a new package (with or
without substitutes), I get the following message:
guix package: error: build failed: getting attributes of path
`/nix/store/fcwh19ljibqjfx0c3cwnwcc7p31aq227-glibc-2.17-locales': No such file
or directory
I’ve already tried to run ‘guix gc’, but it didn’t help.
diff --git a/guix/scripts/package.scm b/guix/scripts/package.scm
index 1393ca3..6e8171c 100644
--- a/guix/scripts/package.scm
+++ b/guix/scripts/package.scm
@@ -34,6 +34,7 @@
#:use-module (ice-9 vlist)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-11)
+ #:use-module (srfi srfi-19)
#:use-module (srfi srfi-26)
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-37)
@@ -246,6 +247,127 @@ all of PACKAGES, a list of name/version/output/path/deps tuples."
(switch-link)))
(else (switch-link) ; anything else
+(define (string->generations str)
+ "Return a list of generations matching a pattern in STR. This function
+accepts the following patterns: \"1\", \"1,2,3\", \"1..9\", \"1..\", \"..9\"."
+ (define (maybe-integer)
+(let ((x (string->number str)))
+ (and (integer? x)
+ x)))
+
+ (define (maybe-comma-separated-integers)
+(let ((lst (delete-duplicates
+(map string->number
+ (delete "" (string-split str #\,))
+ (and (every integer? lst)
+ lst)))
+
+ (cond ((maybe-integer)
+ =>
+ list)
+((maybe-comma-separated-integers)
+ =>
+ identity)
+((string-match "^([0-9]+)\\.\\.([0-9]+)$" str)
+ =>
+ (lambda (match)
+ (let ((s (string->number (match:substring match 1)))
+ (e (string->number (match:substring match 2
+ (and (every integer? (list s e))
+ (<= s e)
+ (iota (1+ (- e s)) s)
+((string-match "^([0-9]+)\\.\\.$" str)
+ =>
+ (lambda (match)
+ (let ((s (string->number (match:substring match 1
+ (and (integer? s)
+ `(>= ,s)
+((string-match "^\\.\\.([0-9]+)$" str)
+ =>
+ (lambda (match)
+ (let ((e (string->number (match:substring match 1
+ (and (integer? e)
+ `(<= ,e)
+(else #f)))
+
+(define (string->duration str)
+ "Return a duration matching a pattern in STR. This function accepts the
+following patterns: \"1d\", \"1w\", \"1m\"."
+ (define (hours->duration hours match)
+(make-time time-duration 0
+ (* 3600 hours (string->number (match:substring match 1)
+
+ (cond ((string-match "^([0-9]+)d$" str)
+ =>
+ (lambda (match)
+ (hours->duration 24 match)))
+((string-match "^([0-9]+)w$" str)
+ =>
+ (lambda (match)
+ (hours->duration (* 24 7) match)))
+((string-match "^([0-9]+)m$" str)
+ =>
+ (lambda (match)
+ (hours->duration (* 24 30) match)))
+(else #f)))
+
+(define* (available-generations str #:optional (profile %current-profile))
+ "Return a list of available generations matching pattern in STR. See
+'string->generations' and 'string->duration' for a list of valid patterns."
+ (define (valid-generations lst)
+(define (valid-generation? n)
+ (any (cut = n <>) (generation-numbers profile)))
+
+(fold-right (lambda (x acc)
+ (if (valid-generation? x)
+ (cons x acc)
+ acc))
+'()
+lst))
+
+ (define (filter-generations generations)
+(match generations
+ (() '())
+ (('>= n)
+ (drop-while (cut > n <>)
+ (generation-numbers profile)))
+ (('<= n)
+ (valid-generations (iota n 1)))
+ ((lst ..1)
+ (valid-generations lst))
+ (_ #f)))
+
+ (define (filter-by-duration duration)
+(define dates-generations
+ ;; Return an alist of dates and generations.
+ (map (lambda (x)
+ (cons (and=> (stat (format #f "~a-~a-link"
+profile (number->string x)))
+ stat:ctime)
+ x))
+ (generation-numbers profile)))
+
+(define dates
+ (fold-right (lambda (x acc)
+(cons (first x) acc))
+ '()
+ dates-generations))
+
+(match duration
+ (#f #f)
+ (res
+ (let ((s (time-second (subtract-duration (current-time) duration
+
Re: New ‘--list-generations’ and ‘--delete-generations’ options
Nikita Karetnikov skribis: >> Probably this can reduce to a big ‘cond’, which would be even more >> readable: > >> (cond ((maybe-integer) >> => >> list) >> ((string-match "^([0-9]+)\\.\\.([0-9]+)$" str) >> => >> (lambda (match) >>...)) >> ...) > > Are you sure? I haven’t found a way to make ‘cond’ as readable as ‘or’. Yes, it makes it easier to enumerate all the cases, and to reason about it. For instance, in ‘string->generations’, getting rid of ‘maybe-*-range’ and instead inlining the ‘string-match’ calls in ‘cond’ would greatly clarify things IMO: (cond ((maybe-integer) ...) ((maybe-comma-separated-integers) ...) ((string-match p1 x) => ...) ((string-match p2 x) => ...) ((string-match p3 x) => ...) (else #f)) > I’m attaching a sketchy version. If you don’t see any problems, I’ll > try to integrate this code into ‘package.scm’. I’d prefer clearer case analysis as shown above. Thanks, Ludo’.
Re: New ‘--list-generations’ and ‘--delete-generations’ options
> By definition submatches 1 and 2 exist when RES is true.
> Thus, I’d remove ‘safe-match:substring->number’ and do:
> (match (string-match "^([0-9]+)\\.\\.([0-9]+)$" str)
> (#f #f)
> (matches
> (let ((start (number->string (match:substring matches 1)))
>(end (number->string (match:substring matches 2
>...)))
Done.
> Probably this can reduce to a big ‘cond’, which would be even more
> readable:
> (cond ((maybe-integer)
> =>
> list)
> ((string-match "^([0-9]+)\\.\\.([0-9]+)$" str)
> =>
> (lambda (match)
>...))
> ...)
Are you sure? I haven’t found a way to make ‘cond’ as readable as ‘or’.
I’m attaching a sketchy version. If you don’t see any problems, I’ll
try to integrate this code into ‘package.scm’.
(Something is wrong with the store on my machine, so I can’t properly
test the filtering part. But I’ll do it as soon as possible.)
(define-module (avail-generations)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-19)
#:use-module (srfi srfi-26)
#:use-module (ice-9 regex)
#:use-module (ice-9 match))
(define profile-numbers (@@ (guix scripts package) profile-numbers))
(define %current-profile (@@ (guix scripts package) %current-profile))
;;;
;;; Parsing.
;;;
(define (string->generations str)
(define (maybe-integer)
(let ((x (string->number str)))
(and (integer? x)
(list x
(define (maybe-comma-separated-integers)
(let ((lst (delete-duplicates
(map string->number
(delete "" (string-split str #\,))
(and (every integer? lst)
lst)))
(define (maybe-whole-range)
(match (string-match "^([0-9]+)\\.\\.([0-9]+)$" str)
(#f #f)
(res
(let ((s (string->number (match:substring res 1)))
(e (string->number (match:substring res 2
(and (every integer? (list s e))
(<= s e)
(iota (1+ (- e s)) s))
(define (maybe-start-range)
(match (string-match "^([0-9]+)\\.\\.$" str)
(#f #f)
(res
(let ((s (string->number (match:substring res 1
(and (integer? s)
`(>= ,s))
(define (maybe-end-range)
(match (string-match "^\\.\\.([0-9]+)$" str)
(#f #f)
(res
(let ((e (string->number (match:substring res 1
(and (integer? e)
`(<= ,e))
(or (maybe-integer) (maybe-comma-separated-integers)
(maybe-whole-range) (maybe-start-range) (maybe-end-range)))
(define (string->duration str)
(define (maybe-duration hours pattern)
(match (string-match pattern str)
(#f #f)
(res
(make-time time-duration 0
(* 3600 hours (string->number (match:substring res 1)))
(define (days)
(maybe-duration 24 "^([0-9]+)d$"))
(define (weeks)
(maybe-duration (* 24 7) "^([0-9]+)w$"))
(define (months)
(maybe-duration (* 24 30) "^([0-9]+)m$"))
(or (days) (weeks) (months)))
;;;
;;; Filtering.
;;;
(define* (available-generations str #:optional (profile %current-profile))
(define (valid-generations lst)
(define (valid-gen? n)
(any (cut = n <>) (profile-numbers profile)))
(fold-right (lambda (x lst)
(if (valid-gen? x)
(cons x lst)
lst))
'()
lst))
;; XXX: find a better name for this function.
(define (filter-generations gens)
(match gens
(() '())
(('>= n)
(drop-while (cut > n <>)
;; XXX: is it really necessary to sort? Check
;; 'profile-numbers'.
(sort (profile-numbers profile) <)))
(('<= n)
(valid-generations (iota n 1)))
((lst ..1)
(valid-generations lst))
(_ #f)))
;; XXX: find a better name.
(define (filter-by-duration dur)
(define dates-gens
;; Return an alist of dates and generations.
(map (lambda (x)
(cons (and=> (stat (format #f "~a-~a-link"
;; XXX: Should I check that
;; 'number->string's argument is
;; actually a number? Can I
;; trust 'profile-numbers'?
profile (number->string x)))
stat:ctime)
x))
;; XXX: Is there a need to sort?
(sort (profile-numbers profile) <)))
(define dates
(fold-right (lambda (x lst)
(cons (first x) lst))
'()
dates-gens))
(match dur
(#f #f)
(res
(let ((s (time-second (subtract-duration (current-time) dur
(map (cut assoc-ref dates-gens <>)
(filter (cut <= s <>) dates))
(cond ((string->generations str)
=>
fi
Re: New ‘--list-generations’ and ‘--delete-generations’ options
Nikita Karetnikov skribis: >>> How can I subtract 22 days from (current-time) using SRFI-19? > >> Note that the above example suggests that ‘string->duration’ returns a >> time object with of type ‘time-duration’ (thus independent of the >> current time.) > > Ah, OK. But we’ll have to subtract from (current-time) later anyway, > right? Why do you want to return a ‘time-duration’ object? To separate concerns, and to simplify testing. > So, here’s the parsing phase. WDYT? Overall looks good to me, but I think it can be simplified: > (define (string->generations str) > (define (maybe-integer) > (let ((x (string->number str))) > (and (integer? x) >(list x > > (define (maybe-comma-separated-integers) > (let ((lst (delete-duplicates > (map string->number > (delete "" (string-split str #\,)) > (and (every integer? lst) >lst))) > (define (safe-match:substring->number match n) > (false-if-exception (string->number (match:substring match n > > (define (maybe-whole-range) > (let* ((rx (make-regexp "^([0-9]+)\\.\\.([0-9]+)$")) >(res (regexp-exec rx str)) >(x (safe-match:substring->number res 1)) >(y (safe-match:substring->number res 2))) By definition submatches 1 and 2 exist when RES is true. Thus, I’d remove ‘safe-match:substring->number’ and do: (match (string-match "^([0-9]+)\\.\\.([0-9]+)$" str) (#f #f) (matches (let ((start (number->string (match:substring matches 1))) (end (number->string (match:substring matches 2 ...))) > (or (maybe-integer) (maybe-comma-separated-integers) > (maybe-whole-range) (maybe-start-range) (maybe-end-range))) Probably this can reduce to a big ‘cond’, which would be even more readable: (cond ((maybe-integer) => list) ((string-match "^([0-9]+)\\.\\.([0-9]+)$" str) => (lambda (match) ...)) ...) > (define (string->duration str) > (define (maybe-duration hours pattern) > (let ((res (regexp-exec (make-regexp pattern) str))) > (false-if-exception >(make-time time-duration 0 > (* 3600 hours (string->number (match:substring res 1))) > > (define (days) > (maybe-duration 24 "^([0-9]+)d$")) > > (define (weeks) > (maybe-duration (* 24 7) "^([0-9]+)w$")) > > (define (months) > (maybe-duration (* 24 30) "^([0-9]+)m$")) > > (or (days) (weeks) (months))) Likewise, just: (define (hours->duration hours) (make-time ...)) (cond ((string-match "^([0-9]+)d$" str) => ...) ...) Thanks, Ludo’.
Re: New ‘--list-generations’ and ‘--delete-generations’ options
>> How can I subtract 22 days from (current-time) using SRFI-19? > Note that the above example suggests that ‘string->duration’ returns a > time object with of type ‘time-duration’ (thus independent of the > current time.) Ah, OK. But we’ll have to subtract from (current-time) later anyway, right? Why do you want to return a ‘time-duration’ object? So, here’s the parsing phase. WDYT? (use-modules (srfi srfi-1) (srfi srfi-19) (ice-9 regex)) ;;; ;;; Parsing. ;;; (define (string->generations str) (define (maybe-integer) (let ((x (string->number str))) (and (integer? x) (list x (define (maybe-comma-separated-integers) (let ((lst (delete-duplicates (map string->number (delete "" (string-split str #\,)) (and (every integer? lst) lst))) (define (safe-match:substring->number match n) (false-if-exception (string->number (match:substring match n (define (maybe-whole-range) (let* ((rx (make-regexp "^([0-9]+)\\.\\.([0-9]+)$")) (res (regexp-exec rx str)) (x (safe-match:substring->number res 1)) (y (safe-match:substring->number res 2))) (and (every integer? (list x y)) (<= x y) (iota (1+ (- y x)) x (define (maybe-start-range) (let* ((rx (make-regexp "^([0-9]+)\\.\\.$")) (res (regexp-exec rx str)) (x (safe-match:substring->number res 1))) (and (integer? x) `(>= ,x (define (maybe-end-range) (let* ((rx (make-regexp "^\\.\\.([0-9]+)$")) (res (regexp-exec rx str)) (x (safe-match:substring->number res 1))) (and (integer? x) `(<= ,x (or (maybe-integer) (maybe-comma-separated-integers) (maybe-whole-range) (maybe-start-range) (maybe-end-range))) (define (string->duration str) (define (maybe-duration hours pattern) (let ((res (regexp-exec (make-regexp pattern) str))) (false-if-exception (make-time time-duration 0 (* 3600 hours (string->number (match:substring res 1))) (define (days) (maybe-duration 24 "^([0-9]+)d$")) (define (weeks) (maybe-duration (* 24 7) "^([0-9]+)w$")) (define (months) (maybe-duration (* 24 30) "^([0-9]+)m$")) (or (days) (weeks) (months))) pgpmILa9DDx6h.pgp Description: PGP signature
Re: New ‘--list-generations’ and ‘--delete-generations’ options
Nikita Karetnikov skribis: >> In that case, would you suggest --list-generations=rec to specify the >> recutils output format (with no filtering)? > > No need for the ‘rec’ part. I propose to output all generations in the > recutils format when ‘--list-generations’ is passed. OK. >> Age specifications would only be of the kind “at least X days/months >> old”. A non-ambiguous syntax is needed, and something more flexible >> than just ‘last-month’. Let’s assume ‘string->duration’: > >> "+22"⇒ # ; 22 days >> "+2w"⇒ # ; 14 days >> "+1m"⇒ # ; 30 days > > The plus sign is confusing (we’re subtracting). I’d rather use ‘22d’, > ‘2w’, and ‘1m’. Fine with me. > How can I subtract 22 days from (current-time) using SRFI-19? Note that the above example suggests that ‘string->duration’ returns a time object with of type ‘time-duration’ (thus independent of the current time.) Subtraction is done like this: --8<---cut here---start->8--- scheme@(guile-user)> (subtract-duration (current-time) (make-time time-duration 0 (* 3600 24 22))) $3 = # scheme@(guile-user)> (date->string (time-utc->date $3)) $4 = "Sun Aug 18 18:54:05+0200 2013" --8<---cut here---end--->8--- HTH, Ludo’.
Re: New ‘--list-generations’ and ‘--delete-generations’ options
> In that case, would you suggest --list-generations=rec to specify the > recutils output format (with no filtering)? No need for the ‘rec’ part. I propose to output all generations in the recutils format when ‘--list-generations’ is passed. > Age specifications would only be of the kind “at least X days/months > old”. A non-ambiguous syntax is needed, and something more flexible > than just ‘last-month’. Let’s assume ‘string->duration’: > "+22"⇒ # ; 22 days > "+2w"⇒ # ; 14 days > "+1m"⇒ # ; 30 days The plus sign is confusing (we’re subtracting). I’d rather use ‘22d’, ‘2w’, and ‘1m’. How can I subtract 22 days from (current-time) using SRFI-19? There is a ‘subtract-duration’ procedure, but I don’t understand how to create a ‘duration’ object. > These are just suggestions, but that seems to make more sense now. Indeed. > Apologies for the confusion! No worries. Thanks for the tips. pgp6J1izKLAVQ.pgp Description: PGP signature
Re: New ‘--list-generations’ and ‘--delete-generations’ options
Nikita Karetnikov skribis: >> I’m asking because if we do that, ‘--list-generations’ may just as well >> print out *all* the generation records. Users who want to select only >> less than one-month old generations can do that with ‘recsel’, and we >> don’t have anything more to do. > >> WDYT? > > I see recutils as an advanced option (for those who need it), not a > replacement for the basic functionality. Yeah, possibly. In that case, would you suggest --list-generations=rec to specify the recutils output format (with no filtering)? >> OTOH, for ‘--delete-generations’ it will still be more convenient to >> support ‘--delete-generations’. > > Could you rephrase? Oops, sorry; this should read: OTOH for ‘--delete-generations’ it will be more convenient to support date-range specifications such as ‘last-month’, etc. > I don’t understand the above. I believe we should think from a user’s > perspective. One should be able to select the needed generations > without relying on a different program. Yes. What about splitting it in two functions: > ‘string->time-range’ → return two SRFI-19 time objects representing a time interval, or #f and #f on failure > ‘generation-within-time-range?’ > Writing tests for the former will be easy. > >> What do you think of the separation I proposed? > > We have the following cases: ‘1’, ‘1,2,3’, ‘1..9’, ‘1..’, ‘..9’, > ‘first-month’, and ‘last-month’. It’s easy to parse the first three and > output a list of generations without checking them. However, you’ll > have to check the profile in the other cases. That’s the problem. [...] > I don’t understand how the ‘string->time-range’ function will help to > solve the above problem. There are only two time-related cases: > ‘first-month’ and ‘last-month’. Why do you want to return a time range > for every case? Could you show an example? Sorry I think I have been sloppy. So we want to support generation enumerations like ‘1,2,3’, ranges (incl. open-ended ranges) like ‘1..9’, and age specifications. For the first one, I would do a ‘string->generations’ procedure: "1,2,3" ⇒ (1 2 3) "1..9" ⇒ (1 2 3 4 5 6 7 8 9) "..9"⇒ (<= 9) ; with the ‘<=’ symbol "1.."⇒ (>= 1) "foo"⇒ #f Age specifications would only be of the kind “at least X days/months old”. A non-ambiguous syntax is needed, and something more flexible than just ‘last-month’. Let’s assume ‘string->duration’: "+22"⇒ # ; 22 days "+2w"⇒ # ; 14 days "+1m"⇒ # ; 30 days Then one just needs to ‘filter’ all the generations that match the specification. This way, there are two parsing procedures, and one or two filtering procedures. These are just suggestions, but that seems to make more sense now. WDYT? Apologies for the confusion! Thanks, Ludo’.
Re: New ‘--list-generations’ and ‘--delete-generations’ options
> I’m asking because if we do that, ‘--list-generations’ may just as well > print out *all* the generation records. Users who want to select only > less than one-month old generations can do that with ‘recsel’, and we > don’t have anything more to do. > WDYT? I see recutils as an advanced option (for those who need it), not a replacement for the basic functionality. > OTOH, for ‘--delete-generations’ it will still be more convenient to > support ‘--delete-generations’. Could you rephrase? I don’t understand the above. I believe we should think from a user’s perspective. One should be able to select the needed generations without relying on a different program. > I think you find it difficult to test because the parsing and generation > enumeration are intermingled. Right, I agree. >>> What about splitting it in two functions: >>> ‘string->time-range’ → return two SRFI-19 time objects representing a >>> time interval, or #f and #f on failure >>> ‘generation-within-time-range?’ >>> Writing tests for the former will be easy. > What do you think of the separation I proposed? We have the following cases: ‘1’, ‘1,2,3’, ‘1..9’, ‘1..’, ‘..9’, ‘first-month’, and ‘last-month’. It’s easy to parse the first three and output a list of generations without checking them. However, you’ll have to check the profile in the other cases. That’s the problem. It’s also possible to write something like this (untested): (define (available-generations str) (define (integer?*) (integer? (string->number str))) (define (comma-separated-integers?) (every integer? (delete-duplicates (map string->number (delete "" (string-split str #\,)) (define (safe-match:substring->number match n) (false-if-exception (string->number (match:substring match n (define (maybe-whole-range-lst) (let* ((rx (make-regexp "^([0-9]+)\\.\\.([0-9]+)$")) (res (regexp-exec rx str)) (x (safe-match:substring->number res 1)) (y (safe-match:substring->number res 2))) (list x y))) (define (whole-range?) (let ((x (first maybe-whole-range-lst)) (y (last maybe-whole-range-lst))) (and (every integer? (maybe-whole-range-lst)) (<= x y (define (maybe-start-range) (let* ((rx (make-regexp "^([0-9]+)\\.\\.$")) (res (regexp-exec rx str))) (safe-match:substring->number res 1))) (define (start-range?) (integer? (maybe-start-range))) ;; ... ) First, this is quite wordy. Moreover, we’ll have to move every definition to the global namespace if we want to test only syntax. I doubt that the above is the way to go. I don’t understand how the ‘string->time-range’ function will help to solve the above problem. There are only two time-related cases: ‘first-month’ and ‘last-month’. Why do you want to return a time range for every case? Could you show an example? pgpPfOC9czbab.pgp Description: PGP signature
Re: New ‘--list-generations’ and ‘--delete-generations’ options
Nikita Karetnikov skribis: >> BTW, what did you think of the idea of using recutils format as the >> output? (Either as the sole output format, or otherwise as a secondary >> format.) > > I like the idea. It’s always better to use a documented format, > especially when it comes with a mode for Emacs. And don’t forget that > ‘--search’ already uses recutils. I didn’t say anything before because > I haven’t tried to implement this part yet. I’m asking because if we do that, ‘--list-generations’ may just as well print out *all* the generation records. Users who want to select only less than one-month old generations can do that with ‘recsel’, and we don’t have anything more to do. WDYT? OTOH, for ‘--delete-generations’ it will still be more convenient to support ‘--delete-generations’. >>> Do you see any problems? Please check everything (especially the >>> ‘first-month’ and ‘last-month’ functions). > >> Better yet: write test cases. :-) > > I have some tests, but you have to modify ‘int’ and the other related > procedures to use them. So it’s not an option. > > I’m also not sure what’s the best way to test the ‘first-month’ and > ‘last-month’ functions (the validation part). Any ideas? I think you find it difficult to test because the parsing and generation enumeration are intermingled. If parsing is separated as I suggested, with a ‘string->date-range’ procedure, then it becomes trivial to write test cases for that. >> The code otherwise looks OK, but disentangling parsing from validation >> will make it even more pleasant IMO. > > I agree. I just haven’t found a way that avoids unnecessary repetition. What do you think of the separation I proposed? > Could you share your thoughts on other things that are marked with > “XXX”? I don’t have much to say on these at this stage, but I think it’d be easier to comment on the next version of the patch. :-) Thanks, Ludo’.
Re: New ‘--list-generations’ and ‘--delete-generations’ options
> BTW, what did you think of the idea of using recutils format as the > output? (Either as the sole output format, or otherwise as a secondary > format.) I like the idea. It’s always better to use a documented format, especially when it comes with a mode for Emacs. And don’t forget that ‘--search’ already uses recutils. I didn’t say anything before because I haven’t tried to implement this part yet. >> Do you see any problems? Please check everything (especially the >> ‘first-month’ and ‘last-month’ functions). > Better yet: write test cases. :-) I have some tests, but you have to modify ‘int’ and the other related procedures to use them. So it’s not an option. I’m also not sure what’s the best way to test the ‘first-month’ and ‘last-month’ functions (the validation part). Any ideas? > The code otherwise looks OK, but disentangling parsing from validation > will make it even more pleasant IMO. I agree. I just haven’t found a way that avoids unnecessary repetition. (I’ll comment on other issues later.) Could you share your thoughts on other things that are marked with “XXX”? pgpfnOS0ga2uS.pgp Description: PGP signature
Re: New ‘--list-generations’ and ‘--delete-generations’ options
Nikita Karetnikov skribis: > The attached procedure will be invoked when either option is called with > an argument. Nice. BTW, what did you think of the idea of using recutils format as the output? (Either as the sole output format, or otherwise as a secondary format.) > Do you see any problems? Please check everything (especially the > ‘first-month’ and ‘last-month’ functions). Better yet: write test cases. :-) > ;; XXX: (avail-generations "") returns () (because of (csi)). This case > ;; should be handled by a different procedure. Basically, it means that no > ;; arguments were passed to '--list-generations' or '--delete-generations'. > (define* (avail-generations str #:optional (profile %current-profile)) Please, never use abbreviations in public identifiers, and avoid them in private identifiers too (‘valid-generation?’, ‘maybe-integer’ instead of ‘int’, etc.) > "Return a list of generations matching the pattern in STR." What about splitting it in two functions: ‘string->time-range’ → return two SRFI-19 time objects representing a time interval, or #f and #f on failure ‘generation-within-time-range?’ Writing tests for the former will be easy. The code otherwise looks OK, but disentangling parsing from validation will make it even more pleasant IMO. Thanks, Ludo’.
Re: New ‘--list-generations’ and ‘--delete-generations’ options
The attached procedure will be invoked when either option is called with an argument. It returns an empty list if the argument is not valid. Or when the needed generation can’t be found. Do you see any problems? Please check everything (especially the ‘first-month’ and ‘last-month’ functions). (use-modules (srfi srfi-1) (srfi srfi-11) (srfi srfi-26) (ice-9 regex) (ice-9 optargs)) (define profile-numbers (@@ (guix scripts package) profile-numbers)) (define %current-profile (@@ (guix scripts package) %current-profile)) ;; XXX: (avail-generations "") returns () (because of (csi)). This case ;; should be handled by a different procedure. Basically, it means that no ;; arguments were passed to '--list-generations' or '--delete-generations'. (define* (avail-generations str #:optional (profile %current-profile)) "Return a list of generations matching the pattern in STR." (define (valid-gen? n) ;; Is N a valid generation number? (any (cut = n <>) (profile-numbers profile))) (define (valid-gens lst) ;; Return a list of valid generation numbers. (fold-right (lambda (x lst) (if (valid-gen? x) (cons x lst) lst)) '() lst)) (define (int) ;; Does STR contain an integer? (let ((x (string->number str))) (and (integer? x) (valid-gen? x) (list x (define (csi) ;; Does STR contain comma-separated integers? ;; XXX: Should it handle spaces? ;; ;; (let* ((str* (string-concatenate (string-split str #\space))) ;;(lst (map string->number (delete "" (string-split str* #\,) ;; ;; The uncommented version returns '() for "1,2 ", "2, 3", "2 ,3", etc. ;; (The other procedures don't handle similar cases too.) (let ((lst (delete-duplicates (map string->number (delete "" (string-split str #\,)) (and (every integer? lst) (valid-gens lst (define (safe-match:substring->number match n) (false-if-exception (string->number (match:substring match n (define (whole-range) (let* ((rx (make-regexp "^([0-9]+)\\.\\.([0-9]+)$")) (res (regexp-exec rx str)) (x (safe-match:substring->number res 1)) (y (safe-match:substring->number res 2))) (and (every integer? (list x y)) (<= x y) ; in Haskell, [1..1] => [1] (valid-gens (iota (1+ (- y x)) x) (define (start-range) (let* ((rx (make-regexp "^([0-9]+)\\.\\.$")) (res (regexp-exec rx str)) (x (safe-match:substring->number res 1))) (and (integer? x) (drop-while (cut > x <>) ;; XXX: Is it really necessary to sort? (sort (profile-numbers profile) <) (define (end-range) (let* ((rx (make-regexp "^\\.\\.([0-9]+)$")) (res (regexp-exec rx str)) (x (safe-match:substring->number res 1))) (and (integer? x) (valid-gens (iota x 1) (define dates-gens ;; Return an alist of dates and generations. (map (lambda (x) (cons (and=> (stat (format #f "~a-~a-link" ;; XXX: Should I check that ;; 'number->string's argument is ;; actually a number? Can I ;; trust 'profile-numbers'? profile (number->string x))) stat:ctime) x)) ;; XXX: Is there a need to sort? (sort (profile-numbers profile) <))) (define dates (fold-right (lambda (x lst) (cons (first x) lst)) '() dates-gens)) (define (first-month) (let ((x (+ (apply min dates) (* 30 86400 ; add 30 days (and (string=? "first-month" str) (map (cut assoc-ref dates-gens <>) (filter (cut >= x <>) dates) (define (last-month) (let ((x (- (apply max dates) (* 30 86400 ; subtract 30 days (and (string=? "last-month" str) (map (cut assoc-ref dates-gens <>) (filter (cut <= x <>) dates) (or (int) (csi) (whole-range) (start-range) (end-range) (first-month) (last-month) '())) ;;; ;;; Valid syntax. ;;; (for-each (lambda (x) (display (avail-generations x)) (newline)) (list "1" "6" "12" "3," "4,4" "2,3" "4,5,1,2" "3,2,3," "1..3" "2..4" "1..11" "3..3" "12..12" "1.." "3.." "13.." "..1" "..7" "..14" "first-month" "last-month")) pgpPfHfEKWhhz.pgp Description: PGP signature
Re: New ‘--list-generations’ and ‘--delete-generations’ options
Nikita Karetnikov skribis: > I’m trying to handle the “last-month” and the “first-month” cases. I’d > like to use ‘profile-numbers’* to construct an alist of generations and > their creation dates. > > What can I use to get the creation date of a file? I can’t find > anything in the manual. Something like (and=> (stat foo) stat:ctime). > * We should rename it to ‘generation-numbers’ or something like that. Probably, yes. Ludo’.
