Re: Linting, and how to get the information in to the Guix Data Serivce

[Christopher Baines]

Ludovic Courtès <address@hidden> writes:

> Hello!
>
> Christopher Baines <address@hidden> skribis:
>
>> Christopher Baines <address@hidden> 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 <address@hidden>
>> 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 <lint-warning>
>> records directly, rather than having to parse the output to determine the
>> package and location.
>
> [...]
>
>> +(define-record-type* <lint-warning>
>> +  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
<lint-warnings> will make writing the tests easier, which is nice.

Thanks,

Chris

signature.asc
Description: PGP signature