Ludovic Courtès writes: > Hello! > > Christopher Baines skribis: > >> Christopher Baines writes: >> >>> I've never worked with this part of Guix before, and some of it is quite >>> complex, so I've started by attempting to do the first bit, storing >>> warnings as data before outputting them. I've attached a patch. > > Providing more structure sounds like a good idea to me (though in the > end it’s still just text; not sure if that’s good enough for what you > had in mind?). Great, I'm also not entirely sure what would be best, but I think having the package and location as the record values, along with the message is a good start. > From a UI viewpoint, it’s important that ‘guix lint’ prints warning as > they come, rather than eat CPU for some time and eventually spit out > everything at once. So, currently this does change the behaviour, but I'm hoping it's still ok. The warnings will be printed after running each checker. So if there are checkers that take a long time, and possibly report multiple warnings, then that could be an issue, but I don't believe this is the case. >> From cd16443893afdacf9f3e4d8256cc943a5928aed4 Mon Sep 17 00:00:00 2001 >> From: Christopher Baines >> Date: Mon, 6 May 2019 19:00:58 +0100 >> Subject: [PATCH] scripts: lint: Handle warnings with a record type. >> >> Rather than emiting warnings directly to a port, have the checkers return the >> warning or warnings. >> >> This makes it easier to use the warnings in different ways, for example, >> loading the data in to a database, as you can work with the >> records directly, rather than having to parse the output to determine the >> package and location. > > [...] > >> +(define-record-type* >> + lint-warning make-lint-warning >> + lint-warning? >> + (package lint-warning-package) >> + (message lint-warning-message) >> + (location lint-warning-field >> + (default #f))) > > It could be useful to have a ‘checker’ field linking to the checker that > produced the warning. Good point. Any suggestions on how to do this? At the moment, the checker values don't really have names, they're just in a list. I could move the definitions out of the list, and define them at the top level like packages. Another option I was possibly considering is populating the checker field after the warnings are returned from the checker function... any thoughts? >> (define (check-not-empty description) >> (when (string-null? description) >> - (emit-warning package >> + (make-warning package >> (G_ "description should not be empty") >> - 'description))) >> + #:field 'description))) > > This procedure returns either a warning or the unspecified value, which > is probably not intended? I suppose a lot of the code is currently > written with ‘emit-warning’ in effect position, which will have to be > changed. So, it's important that all the generated warnings are returned, so I changed some checkers to return lists. Also, I added a append-warnings function which takes 1 or more arguments, and will sort the warnings in to a neat list. I did consider changing all the checkers to return a neat list of warnings, but that would require more changes. What do you think? > I suppose tests/lint.scm needs to be converted to the new API? Ah, yep. I've made a start on this now. I think returning will make writing the tests easier, which is nice. Thanks, Chris