[PATCH] Fix clocktable scope parameter

[PATCH] Fix clocktable scope parameter

[Eduardo Bellani]

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


org-clock.el: Fix clocktable scope parameter

* 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 adds back to the clocktable the capacity to have as scope both a
list of file paths or a function that returns such a list.

---
 lisp/org-clock.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index 65c13fd..18bb443 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -2370,7 +2370,8 @@ the currently selected interval size."
 		    (`file-with-archives
 		     (and buffer-file-name
 			  (org-add-archive-files (list buffer-file-name))))
-		    ((pred consp) scope)
+                    ((and (pred #'listp) (pred (lambda (scope) (symbolp (car scope))))) (eval scope))
+                    ((pred listp) scope)
 		    (_ (or (buffer-file-name) (current-buffer)))))
 	   (block (plist-get params :block))
 	   (ts (plist-get params :tstart))
--

TINYCHANGE

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

Re: [PATCH] Fix clocktable scope parameter

[Nicolas Goaziou]

Hello,

Eduardo Bellani <ebellani@gmail.com> writes:

> org-clock.el: Fix clocktable scope parameter
>
> * 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 adds back to the clocktable the capacity to have as scope both a
> list of file paths or a function that returns such a list.

Thank you.

However, I'd rather not introduce more `eval' in the code base.

Instead, you can test if scope is a function and `funcall' it.  This is
a new feature, which would require some documentation and an entry in
ORG-NEWS.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Fix clocktable scope parameter

[Eduardo Bellani]

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

Hello Nicolas,

I agree with you about introducing 'eval' into the code base. This was
just me getting the code from 8.3 back in a format the current code
understand. Which leads me to conclude that this is not new
functionality, just old undocumented functionality. I've found out about
it through this SE post: http://emacs.stackexchange.com/a/7903

Anyway, I'll modify the patch and include documentation for this
functionality.

Nicolas Goaziou writes:

> Hello,
>
> Eduardo Bellani <ebellani@gmail.com> writes:
>
>> org-clock.el: Fix clocktable scope parameter
>>
>> * 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 adds back to the clocktable the capacity to have as scope both a
>> list of file paths or a function that returns such a list.
>
> Thank you.
>
> However, I'd rather not introduce more `eval' in the code base.
>
> Instead, you can test if scope is a function and `funcall' it.  This is
> a new feature, which would require some documentation and an entry in
> ORG-NEWS.
>
> Regards,

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

Re: [PATCH] Fix clocktable scope parameter

[Eduardo Bellani]

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

Hello everyone,

One extra question, about documentation.

The place for documenting this feature seems to be 'orgmanual.org'. But
my searches revealed that there is also 'org.texi' there. Exporting the
first file to the texi format didn't match the second file.

Is there a documentation about, well, adding documentation?

Thanks.

Nicolas Goaziou writes:

> Hello,
>
> Eduardo Bellani <ebellani@gmail.com> writes:
>
>> org-clock.el: Fix clocktable scope parameter
>>
>> * 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 adds back to the clocktable the capacity to have as scope both a
>> list of file paths or a function that returns such a list.
>
> Thank you.
>
> However, I'd rather not introduce more `eval' in the code base.
>
> Instead, you can test if scope is a function and `funcall' it.  This is
> a new feature, which would require some documentation and an entry in
> ORG-NEWS.
>
> Regards,

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

Re: [PATCH] Fix clocktable scope parameter

[Thomas S. Dye]

Aloha Eduardo,

orgmanual.org is an old experiment about keeping the org mode manual in
org mode.

The documentation is in org.texi, and this is the one you should work on
for new documentation.

All the best,
Tom

Eduardo Bellani writes:

> Hello everyone,
>
> One extra question, about documentation.
>
> The place for documenting this feature seems to be 'orgmanual.org'. But
> my searches revealed that there is also 'org.texi' there. Exporting the
> first file to the texi format didn't match the second file.
>
> Is there a documentation about, well, adding documentation?
>
> Thanks.
>
> Nicolas Goaziou writes:
>
>> Hello,
>>
>> Eduardo Bellani <ebellani@gmail.com> writes:
>>
>>> org-clock.el: Fix clocktable scope parameter
>>>
>>> * 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 adds back to the clocktable the capacity to have as scope both a
>>> list of file paths or a function that returns such a list.
>>
>> Thank you.
>>
>> However, I'd rather not introduce more `eval' in the code base.
>>
>> Instead, you can test if scope is a function and `funcall' it.  This is
>> a new feature, which would require some documentation and an entry in
>> ORG-NEWS.
>>
>> Regards,


-- 
Thomas S. Dye
http://www.tsdye.com

Re: [PATCH] Fix clocktable scope parameter

[Eduardo Bellani]

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

org-clock.el: Fix clocktable scope parameter

* 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.

* doc/org.texi: Document the feature of using a function as the scope
  for the clocktable.

This adds back to the clocktable the capacity to have as scope both a
list of file paths or a function that returns such a list.
---
 doc/org.texi      | 1 +
 lisp/org-clock.el | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/doc/org.texi b/doc/org.texi
index 6be76d8..51bd702 100644
--- a/doc/org.texi
+++ b/doc/org.texi
@@ -6638,6 +6638,7 @@ be selected:
              tree       @r{the surrounding level 1 tree}
              agenda     @r{all agenda files}
              ("file"..) @r{scan these files}
+             (function-name) @r{scan the list of files returned by calling this function.}
              file-with-archives    @r{current file and its archives}
              agenda-with-archives  @r{all agenda files, including archives}
 :block       @r{The time block to consider.  This block is specified either}
diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index 65c13fd..2f7db65 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -2370,7 +2370,9 @@ the currently selected interval size."
 		    (`file-with-archives
 		     (and buffer-file-name
 			  (org-add-archive-files (list buffer-file-name))))
-		    ((pred consp) scope)
+                    ((and (pred #'listp) (pred (lambda (scope) (symbolp (car scope)))))
+		     (funcall (car scope)))
+                    ((pred listp) scope)
 		    (_ (or (buffer-file-name) (current-buffer)))))
 	   (block (plist-get params :block))
 	   (ts (plist-get params :tstart))
-- 
TINYCHANGE


Thomas S. Dye writes:

> Aloha Eduardo,
>
> orgmanual.org is an old experiment about keeping the org mode manual in
> org mode.
>
> The documentation is in org.texi, and this is the one you should work on
> for new documentation.
>
> All the best,
> Tom
>
> Eduardo Bellani writes:
>
>> Hello everyone,
>>
>> One extra question, about documentation.
>>
>> The place for documenting this feature seems to be 'orgmanual.org'. But
>> my searches revealed that there is also 'org.texi' there. Exporting the
>> first file to the texi format didn't match the second file.
>>
>> Is there a documentation about, well, adding documentation?
>>
>> Thanks.
>>
>> Nicolas Goaziou writes:
>>
>>> Hello,
>>>
>>> Eduardo Bellani <ebellani@gmail.com> writes:
>>>
>>>> org-clock.el: Fix clocktable scope parameter
>>>>
>>>> * 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 adds back to the clocktable the capacity to have as scope both a
>>>> list of file paths or a function that returns such a list.
>>>
>>> Thank you.
>>>
>>> However, I'd rather not introduce more `eval' in the code base.
>>>
>>> Instead, you can test if scope is a function and `funcall' it.  This is
>>> a new feature, which would require some documentation and an entry in
>>> ORG-NEWS.
>>>
>>> Regards,


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

Re: [PATCH] Fix clocktable scope parameter

[Nicolas Goaziou]

Hello,

"Thomas S. Dye" <tsd@tsdye.com> writes:

> orgmanual.org is an old experiment about keeping the org mode manual in
> org mode.

I  don't consider it as an outdated experiment however. I welcome any
work to keep it up to date.

> The documentation is in org.texi, and this is the one you should work on
> for new documentation.

Agreed.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Fix clocktable scope parameter

[Nicolas Goaziou]

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,

-- 
Nicolas Goaziou

Re: [PATCH] Fix clocktable scope parameter

[Eduardo Bellani]

[-- 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 --]

Re: [PATCH] Fix clocktable scope parameter

[Nicolas Goaziou]

Hello,

Eduardo Bellani <ebellani@gmail.com> writes:

> 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?

On the contrary, I think the above is a bit ambiguous, and doesn't bring
anything, since

  :scope (foo bar baz)

can also be written, if really needed,

  :scope (lambda () (foo bar baz))

I'd favor clarity here and suggest to accept a function of no argument.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Fix clocktable scope parameter

[Eduardo Bellani]

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

Hello

Nicolas Goaziou writes:

> can also be written, if really needed,
>
>   :scope (lambda () (foo bar baz))
>
> I'd favor clarity here and suggest to accept a function of no argument.

The only problem I see with this is that it will be backwards
incompatible.

Any thoughts on that?

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

Re: [PATCH] Fix clocktable scope parameter

[Nicolas Goaziou]

Eduardo Bellani <ebellani@gmail.com> writes:

> The only problem I see with this is that it will be backwards
> incompatible.
>
> Any thoughts on that?

That's not a problem, as long as there is an entry about it in ORG-NEWS
and that the behaviour is properly documented this time.

Regards,

Re: [PATCH] Fix clocktable scope parameter

[Eduardo Bellani]

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

org-clock.el: Fix clocktable scope parameter

* lisp/org-clock.el (org-dblock-write:clocktable): Funcall the scope
  argument if it is a function.

* doc/org.texi: Document the feature of using a function, both bounded
  and as a lambda form, as the scope for the clocktable.

This modifies the behavior of the scope parameter to have as scope both
a list of file paths or a nullary function that returns such a list.
---
 doc/org.texi      | 2 ++
 lisp/org-clock.el | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/doc/org.texi b/doc/org.texi
index 6be76d8..ef4021f 100644
--- a/doc/org.texi
+++ b/doc/org.texi
@@ -6638,6 +6638,8 @@ be selected:
              tree       @r{the surrounding level 1 tree}
              agenda     @r{all agenda files}
              ("file"..) @r{scan these files}
+             symbol     @r{scan the files returned by calling the nullary function bound to this symbol}
+             lambda-form  @r{Same as above, but the nullary function is passed as a lambda form.}
              file-with-archives    @r{current file and its archives}
              agenda-with-archives  @r{all agenda files, including archives}
 :block       @r{The time block to consider.  This block is specified either}
diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index 65c13fd..1a3f3d6 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -2370,7 +2370,8 @@ the currently selected interval size."
 		    (`file-with-archives
 		     (and buffer-file-name
 			  (org-add-archive-files (list buffer-file-name))))
-		    ((pred consp) scope)
+                    ((pred #'functionp) (funcall scope))
+                    ((pred listp) scope)
 		    (_ (or (buffer-file-name) (current-buffer)))))
 	   (block (plist-get params :block))
 	   (ts (plist-get params :tstart))
--
TINYCHANGE

Nicolas Goaziou writes:

> Eduardo Bellani <ebellani@gmail.com> writes:
>
>> The only problem I see with this is that it will be backwards
>> incompatible.
>>
>> Any thoughts on that?
>
> That's not a problem, as long as there is an entry about it in ORG-NEWS
> and that the behaviour is properly documented this time.
>
> Regards,

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

Re: [PATCH] Fix clocktable scope parameter

[Nicolas Goaziou]

Hello,


Thank you. Some comments follow.

Eduardo Bellani <ebellani@gmail.com> writes:

> org-clock.el: Fix clocktable scope parameter

Why is it a "fix"? I think this patch belongs to master branch, not
maint.

> * lisp/org-clock.el (org-dblock-write:clocktable): Funcall the scope
>   argument if it is a function.
>
> * doc/org.texi: Document the feature of using a function, both bounded
>   and as a lambda form, as the scope for the clocktable.

Since a bounded symbol and a lambda form are both functions, I'd rather
not distinguish them. Actually, this distinction is usually not made.
> +             symbol     @r{scan the files returned by calling the nullary function bound to this symbol}
> +             lambda-form  @r{Same as above, but the nullary function is passed as a lambda form.}

See above. I suggest:

  function @r{...}

> -		    ((pred consp) scope)
> +                    ((pred #'functionp) (funcall scope))

  (pred functionp)

> +                    ((pred listp) scope)

Is there any reason to change consp into listp? This change doesn't
appear in the commit message.


Could you provide an entry in ORG-NEWS? Also, would you mind writing
a couple of simple tests in test-org-clock.el?

Regards,

-- 
Nicolas Goaziou