emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
To: "Göktuğ Kayaalp" <self@gkayaalp.com>
Cc: Aaron Ecay <aaronecay@gmail.com>, emacs-orgmode@gnu.org
Subject: Re: Inheriting some local variables from source code block editing buffers
Date: Sat, 19 May 2018 14:26:50 +0200	[thread overview]
Message-ID: <87lgcfhlv9.fsf@nicolasgoaziou.fr> (raw)
In-Reply-To: <ygmwow0ocsa.fsf@gkayaalp.com> ("Göktuğ Kayaalp"'s message of "Sat, 19 May 2018 00:48:53 +0300")

Hello,

Göktuğ Kayaalp <self@gkayaalp.com> 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
> +@samp{edit-bindings} 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
> +@samp{edit-bindings} 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

  reply	other threads:[~2018-05-19 12:27 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-29 17:19 Inheriting some local variables from source code block editing buffers Göktuğ Kayaalp
2018-04-29 22:09 ` Bastien
2018-05-01  3:30   ` Göktuğ Kayaalp
2018-05-01  8:43     ` Nicolas Goaziou
2018-05-01  8:45       ` Nicolas Goaziou
2018-05-01 11:41       ` Göktuğ Kayaalp
2018-05-01 11:55         ` Nicolas Goaziou
     [not found]           ` <ygmvac7lcl1.fsf@gkayaalp.com>
2018-05-01 14:00             ` Nicolas Goaziou
2018-05-01 16:37           ` Aaron Ecay
     [not found]             ` <ygmin87kwua.fsf@gkayaalp.com>
2018-05-01 19:35               ` Aaron Ecay
2018-05-01 20:04                 ` Göktuğ Kayaalp
2018-05-01 20:53                   ` Aaron Ecay
2018-05-01 22:12                     ` Göktuğ Kayaalp
2018-05-01 22:19                       ` Aaron Ecay
2018-05-01 22:26                       ` Göktuğ Kayaalp
2018-05-02 10:16                         ` Nicolas Goaziou
2018-05-02 19:52                           ` Göktuğ Kayaalp
2018-05-01 11:45       ` Göktuğ Kayaalp
2018-05-14  5:44 ` Göktuğ Kayaalp
2018-05-14 12:13   ` Nicolas Goaziou
2018-05-14 16:34     ` Göktuğ Kayaalp
2018-05-14 16:47       ` Nicolas Goaziou
2018-05-15 18:36         ` Göktuğ Kayaalp
2018-05-18 21:48           ` Göktuğ Kayaalp
2018-05-19 12:26             ` Nicolas Goaziou [this message]
2018-05-21 14:20             ` Aaron Ecay
2018-05-14 13:33   ` Aaron Ecay
2018-05-14 16:46     ` Göktuğ Kayaalp

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87lgcfhlv9.fsf@nicolasgoaziou.fr \
    --to=mail@nicolasgoaziou.fr \
    --cc=aaronecay@gmail.com \
    --cc=emacs-orgmode@gnu.org \
    --cc=self@gkayaalp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).