Hello, Göktuğ Kayaalp <s...@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