Re: [PATCH] Fix clocktable scope parameter

[Eduardo Bellani <ebellani@gmail.com>]

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

Hello Nicolas.

> Thank you. However, this is not a "fix" per se.

I agree. I should be more explicit as to what this is.

>> * lisp/org-clock.el (org-dblock-write:clocktable): Make sure to eval
>>   the scope if it is a lisp expression, or to return the scope if it
>>   is just a list.
>
> This comment is no longer accurate.

Actually, I think the above is accurate, but poorly expressed. As I'll
try to show on the next email. But before that, I though of discussing
this a bit here.

The old behavior was an eval on a form if that form was not a list of
strings. The implicit expectation was for a list of strings to be
returned by that eval call.

The above seems to be a raw attempt to evaluate a function form. In that
case, it seems more elegant to be more explicit and do a

     (apply (car scope) (cdr scope))


This also allows for passing arguments to the function without using the
full power of eval.

What do you guys think?

Nicolas Goaziou writes:

> Hello,
>
> Eduardo Bellani <ebellani@gmail.com> writes:
>
>> org-clock.el: Fix clocktable scope parameter
>
> Thank you. However, this is not a "fix" per se.
>
>> * lisp/org-clock.el (org-dblock-write:clocktable): Make sure to eval
>>   the scope if it is a lisp expression, or to return the scope if it
>>   is just a list.
>
> This comment is no longer accurate.
>
>> +             (function-name) @r{scan the list of files returned by calling this function.}
>
> Why should the function be within a list? I suggest something like
>
>   function @r{list of files returned by calling the function with no argument}
>
>> -		    ((pred consp) scope)
>> +                    ((and (pred #'listp) (pred (lambda (scope) (symbolp (car scope)))))
>> +		     (funcall (car scope)))
>
> Per above, it should be
>
>   ((pred functionp) (funcall scope))
>
> Regards,

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 180 bytes --]