From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roel Janssen Subject: Re: Display diffs between generations. Date: Mon, 29 Aug 2016 21:08:44 +0200 Message-ID: <87inuj5r5v.fsf@gnu.org> References: <87eg59izmw.fsf@gnu.org> <87d1kry22u.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:40864) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1beRuh-0006fF-6V for guix-devel@gnu.org; Mon, 29 Aug 2016 15:08:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1beRuf-0005pJ-1t for guix-devel@gnu.org; Mon, 29 Aug 2016 15:08:06 -0400 In-reply-to: <87d1kry22u.fsf@gnu.org> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Ludovic =?utf-8?Q?Court=C3=A8s?= Cc: guix-devel@gnu.org Ludovic Courtès writes: > Hi Roel, > > I’ve just tried the patch and I think it’s awesome! I’ve also always > been dissatisfied with what ‘--list-generations’ provides—it’s > inconvenient as soon as you have more than a handful packages. > > Perhaps we could add an optional argument to --list-generations where: > > --list-generations > > would be equivalent to: > > --list-generations=diff > > and: > > --list-generations=full > > would restore the previous behavior. It doesn’t cost much to do that. > > WDYT? That would be great. Currently, the --list-generations already takes a generation number. How would we deal with the situation where we want to view the differences between generation 2 and 3 only? --list-generations=3,diff That doesn't look appealing to me. Maybe we could do this instead?: --list-generations-diff > I have only minor stylistic comments: > > Roel Janssen skribis: > >> +(define (display-profile-content-diff profile number) > > Or just ‘display-profile-diff’? > >> + "Display the changed packages in PROFILE with generation specified by NUMBER." > > “… compared to generation NUMBER”? > >> + (define (equal-entry? first second) >> + (string= (manifest-entry-item first) >> + (manifest-entry-item second))) >> + >> + (define* (display-entries entries #:optional (prefix " ")) >> + (for-each >> + (match-lambda >> + (($ name version output location _) >> + (format #t " ~a ~a\t~a\t~a\t~a~%" >> + prefix name version output location))) >> + entries)) > > In general, I find it clearer to define the singular form and inline the > ‘for-each’: > > (define display-entry > (match-lambda …)) > > … > > (for-each display-entry entries) > > Otherwise LGTM! Right. That does look clearer. I refactored the function further to the following: ------- BEGIN ------- (define (display-profile-content-diff profile number) "Display the changed packages in PROFILE compared to generation NUMBER." (define (equal-entry? first second) (string= (manifest-entry-item first) (manifest-entry-item second))) (define display-entry (match-lambda (($ name version output location _) (format #f "~a\t~a\t~a\t~a~%" name version output location)))) (define (display-entries entries prefix) (for-each (lambda (entry) (format #t " ~a ~a" prefix (display-entry entry))) entries)) (define (list-entries input) (manifest-entries (profile-manifest (generation-file-name profile input)))) (define (display-diff profile older newer) (if (= older 0) (display-profile-content profile newer) (begin ;; List newly installed packages. (display-entries (lset-difference equal-entry? (list-entries newer) (list-entries older)) "+") ;; List newly removed packages. (display-entries (lset-difference equal-entry? (list-entries older) (list-entries newer)) "-")))) (display-diff profile (previous-generation-number profile number) number)) ------- END ------- The thing with `display-entries' is because I cannot pass two arguments in the function used in the `for-each', and the prefix for `installed' or `removed' differs, I cannot remove it that easily :-). Kind regards, Roel Janssen