Re: [O] Inheriting some local variables from source code block editing buffers

[Nicolas Goaziou]

Hello,

Göktuğ Kayaalp <address@hidden> writes:

> And here it is.  Please find the patch attached.

Thank you. Comments follow.

> I have ran the entire test suite against this, which completed w/
> 6 failures, seemingly unrelated (they fail on revision 58c9bedfd too);

You may be using an old revision. I see no error from tests in HEAD.

> find the list below. I have tried to follow apparent conventions in
> each file w.r.t. the copyright/authors lines, sorry if I modified them
> unnecessarily.

Usually, we do not modify authors lines, which represents the people who
initially wrote the file. No worries, though.

> Subject: [PATCH] Implement edit bindings feature
>
> Enable defining local variable bindings to be applied when editing
> source code.
>
> * lisp/org-src.el (org-src--apply-edit-bindings)
> (org-src--simplify-edit-bindings)
> (org-src--parse-edit-bindings)
> (org-src--edit-bindings-string)
> (org-src--get-edit-bindings-for-subtree)
> (org-src--get-edit-bindings-from-header)
> (org-src--collect-global-edit-bindings)
> (org-src--collect-edit-bindings-for-element): New functions.
> (org-src-apply-risky-edit-bindings): New defcustom.
> (org-src--edit-element):
> * doc/org.texi (Editing source code): Add edit bindings.

"org.texi" no longer exists in master. We write the manual as an Org
file: "org-manual.org".

> +It is possible to define local variable bindings for these buffers using the
> address@hidden element header, the @samp{edit-bindings} buffer
> +property, or the @samp{EDIT_BINDINGS} entry property.  All three can be used
> +together, the bindings from the header override those of the subtree, and
> +they both override the bindings from buffer properties.  The syntax is
> +similar to that of @code{let} varlists, but a sole symbol means the
> +variable's value is copied from the Org mode buffer.  Multiple uses of
> address@hidden headers and buffer properties are supported, and works
> +like @code{let*}.  Entry property @samp{EDIT_BINDINGS} can not be repeated.
> +Below is an example:

It seems you are partly re-inventing the wheel here. Org already has
such mechanism, called Babel headers:

    #+PROPERTY: header-args :edit-bindings '(...)

    :PROPERTIES:
    :HEADER-ARGS: :edit-bindings '(...)
    :END:

    #+header: :edit-bindings '(...)
    ...

Of course, this is currently limited to source blocks and inline source
blocks. However, I think it is better to extend them to your use-case
than rewriting something different, albeit similar.

You already looked at `org-babel-get-src-block-info'. You may want to
start hacking on it and extend it instead of providing your own tools.

>  ** New features
> +*** Set local variables for editing blocks
> +Bindings from =edit-bindings= header and buffer property, and
> +=EDIT_BINDINGS= entry property are applied in Org Src buffers.  For
> +example, when editing the following code block with
> +~org-edit-special~:

AFAIU, these bindings are not used when evaluating the buffer, which may
be surprising. What about calling them :local-bindings instead
of :edit-bindings and handle them in Babel too?

In this case, what would be the difference with, e.g., 

    :var lexical-binding=t

> +(defun org-src--apply-edit-bindings (simplified-bindings)

Could you provide a docstring for the functions?

> +  (pcase-dolist (`(,name . ,value) simplified-bindings)
> +    (let ((prompt-apply
> +        (concat "Setting risky local variable ‘%S’"
> +                " in edit-special buffer, its value is: %S; Continue?"))

You can use \ + \n, i.e., continuation string instead of calling
`concat' each time.

> +       (risky-message "%s risky local variable ‘%S’ in edit-special buffer.")

I suggest to use two different messages instead of doing the above, if
one day we move to proper i18n.

> +       (apply-binding (lambda () (set (make-local-variable name)
> +                                      (eval value)))))
> +      (unless
> +       (and
> +        (risky-local-variable-p name)
> +        (cond ((or (and (eq org-src-apply-risky-edit-bindings 'ask)
> +                        (y-or-n-p (format prompt-apply name value)))
> +                   (eq org-src-apply-risky-edit-bindings 'apply-silent))
> +               (funcall apply-binding))
> +              (org-src-apply-risky-edit-bindings
> +               (prog1
> +                   (funcall apply-binding)
> +                 (message risky-message "Applied" name)))

You probably mean

  (funcall apply-binding)
  (message ...)
  t

which is clearer, IMO.

> +              ((not org-src-apply-risky-edit-bindings)
> +               (message risky-message "Skipped" name))
> +              ((eq org-src-apply-risky-edit-bindings 'skip-silent))
> +              ('else
> +               (user-error
> +                "Unexpected value for ‘%S’, will not apply this or any more 
> bindings."
> +                'org-src-apply-risky-edit-bindings))))

Error messages must not end with a period.

> +     (funcall apply-binding)))))

If the second `cond' branch is used, `apply-binding' is called twice,
which is sub-optimal.

> +(defun org-src--simplify-edit-bindings (raw-bindings)

This function really needs a docstring.

> +(defun org-src--collect-edit-bindings-for-element ()

This is where the re-inventing the wheel part starts.

> +(defun org-src--collect-global-edit-bindings ()

Ditto.

> +  ;; XXX: is setting GRANULARITY to 'element a performance
> +  ;; improvement, and does it cause any problems over just using the
> +  ;; default 'object?

Yes, setting GRANULARITY to `element' is faster than setting it to
`object', but you shouldn't parse the whole buffer in the first place.

> +  ;; Also, is it possible to not have to parse the entire buffer every
> +  ;; time?

Of course.

You usually look for a regexp, e.g., "#+\\+KEYWORD:", then check if
you're really at a keyword with (eq (org-element-type element)
'keyword), then process the keyword.

> +    (cl-loop for varexp in sexp
> +          collect
> +          (pcase varexp
> +            ((pred null)

-> ('nil ...)

> +;; Copyright (C) 2018  Göktuğ Kayaalp
>  ;; Copyright (C) 2012-2015  Le Wang

Actually, copyright is wrong. It belongs to FSF, since tests are
probably going to be included in Emacs, too.


Regards,

-- 
Nicolas Goaziou                                                0x80A93738