Re: [PATCH] emacs: add compatability functions for emacs 23
David Bremner writes: > Mark Walters writes: > >> This is a good point. I think I don't mind too much if they do -- they >> should see it is provided by notmuch-lib if they do describe-function >> etc. But maybe bremner would like to comment? >> >> However, maybe other packages are doing the same. Thus I think we should >> not put in a cut down version of read-char-choice but just include the >> whole command from the emacs24 source. That way if any other package is >> doing the same load order doesn't matter -- we don't stomp on them and >> they don't stomp on us. > > There is a sort of precedent with the package cl-lib.el. Of course it's > not exactly the same since it's mainly providing new names for > functionality that already existed. I'm obviously in no place to object strongly, but I like this approach less the more I think about it. I've had similar things come back to bite me too many times in the past. Here I find Steve Purcell raising the same concern: https://github.com/melpa/melpa/pull/1748#issuecomment-45082451 https://github.com/swift-emacs/swift-mode/issues/10 I'm concerned entirely about surprises. Folks don't expect packages to provide future compatibility functions. Imagine using Emacs 23 and, maybe because you're copy/paste hacking your personal elisp, you come to use setq-local. Or perhaps you use some *other* add-on that uses setq-local but makes no effort to support Emacs 23. Then you decide to move (require 'notmuch) a little lower in your .emacs and *boom*, your config is broken. So I'd rather see this kind of thing: ;; TODO: remove and use setq-local directly when Emacs 23 support is dropped. (if (fboundp 'setq-local) (defalias 'notmuch-setq-local 'setq-local) (defmacro notmuch-setq-local (var val) "Set variable VAR to value VAL in current buffer." `(set (make-local-variable ',var) ,val))) Then just call notmuch-setq-local from notmuch code. That follows the expected convention that packages add symbols under their own namespace. But, as far as prior art, we have examples of other packages patching in a setq-local: https://github.com/flycheck/flycheck/commit/b19bd4fdf118c55a96d72b2b3c034a0393cfcae2 https://github.com/fxbois/web-mode/issues/438 ...and I have to agree with Steve when he says "This isn't the most egregious example, though. :-)" ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] emacs: add compatability functions for emacs 23
Mark Walters writes: > This is a good point. I think I don't mind too much if they do -- they > should see it is provided by notmuch-lib if they do describe-function > etc. But maybe bremner would like to comment? > > However, maybe other packages are doing the same. Thus I think we should > not put in a cut down version of read-char-choice but just include the > whole command from the emacs24 source. That way if any other package is > doing the same load order doesn't matter -- we don't stomp on them and > they don't stomp on us. There is a sort of precedent with the package cl-lib.el. Of course it's not exactly the same since it's mainly providing new names for functionality that already existed. Quoting from the package description: , | This is a forward compatibility package, which provides (a subset of) the | features of the cl-lib package introduced in Emacs-24.3, for use on | previous emacsen. | | Make sure this is installed *late* in your `load-path`, i.e. after Emacs's | built-in .../lisp/emacs-lisp directory, so that if/when you upgrade to | Emacs-24.3, the built-in version of the file will take precedence, otherwise | you could get into trouble (although we try to hack our way around the | problem in case it happens). ` > (As an aside if we do that do we need to include any copyright notice or > similar? -- maybe notmuch-lib.el could include notmuch-compat.el which > would contain all the compatability functions?) We should definitely preserve copyright information (the licensing is the same, so no problem there) >> The only other package I have non-trivial experience working with is >> Gnus, and the practice there was to place portability interfaces in the >> gnus- namespace, and refrain from calling the "native" interfaces >> directly. > > I think this would introduce a lot of clutter, so I would prefer not to > go that route. I don't have strong opinions about that, but note that we're talking about (currently) 5 lines of code. d ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] emacs: add compatability functions for emacs 23
On Thu, 27 Oct 2016, Matt Armstrong wrote: > Mark Walters writes: > >> +;; Compatability functions for emacs 23. >> + >> +(unless (fboundp 'setq-local) >> + (defmacro setq-local (var val) >> +`(set (make-local-variable ',var) ,val))) > > A concern I have with this approach is that it modifies symbols outside > the notmuch namespace. Could people on old Emacsen come to rely on the > newer Emacs interfaces inadvertently, and wonder how they came to be, > etc? This is a good point. I think I don't mind too much if they do -- they should see it is provided by notmuch-lib if they do describe-function etc. But maybe bremner would like to comment? However, maybe other packages are doing the same. Thus I think we should not put in a cut down version of read-char-choice but just include the whole command from the emacs24 source. That way if any other package is doing the same load order doesn't matter -- we don't stomp on them and they don't stomp on us. (As an aside if we do that do we need to include any copyright notice or similar? -- maybe notmuch-lib.el could include notmuch-compat.el which would contain all the compatability functions?) > The only other package I have non-trivial experience working with is > Gnus, and the practice there was to place portability interfaces in the > gnus- namespace, and refrain from calling the "native" interfaces > directly. I think this would introduce a lot of clutter, so I would prefer not to go that route. Best wishes Mark ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] emacs: add compatability functions for emacs 23
Mark Walters writes: > +;; Compatability functions for emacs 23. > + > +(unless (fboundp 'setq-local) > + (defmacro setq-local (var val) > +`(set (make-local-variable ',var) ,val))) A concern I have with this approach is that it modifies symbols outside the notmuch namespace. Could people on old Emacsen come to rely on the newer Emacs interfaces inadvertently, and wonder how they came to be, etc? The only other package I have non-trivial experience working with is Gnus, and the practice there was to place portability interfaces in the gnus- namespace, and refrain from calling the "native" interfaces directly. Outside Gnus, I'm ignorant of what Emacs packages typically do for this problem. I have no strong feelings either way. ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] emacs: add compatability functions for emacs 23
On Tue, 25 Oct 2016, David Bremner wrote: > Mark Walters writes: > >> Some of the recent changes to the emacs code have used functions >> introduced in emacs 24. The functions used are read-char-choice and >> setq-local. This changeset adds compatability functions to >> notmuch-lib so that it should work on emacs 23. >> >> --- >> >> Hi >> >> I tried compiling under emacs 23 recently and noticed that some recent >> changes have used some features introuduced in emacs 24. I think we >> still support emacs 23 so this changeset adds two compatability >> functions. > > Hi Mark; > > Can you give me a recipe to reproduce the failures in emacs23? I only > get warnings when building. Of course I believe that things will go > wrong at some point calling non-existent functions. Hi You can make the read-char-choice fail by setting the fcc header to use insert (the default), locking the database (eg doing notmuch tag --batch), composing and sending a message. The Fcc will fail, which will trigger the error. For the setq-local start to compose a message, and then run M-x notmuch-address-toggle-internal-completion This will give the error. Best wishes Mark ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] emacs: add compatability functions for emacs 23
Mark Walters writes: > Some of the recent changes to the emacs code have used functions > introduced in emacs 24. The functions used are read-char-choice and > setq-local. This changeset adds compatability functions to > notmuch-lib so that it should work on emacs 23. > > --- > > Hi > > I tried compiling under emacs 23 recently and noticed that some recent > changes have used some features introuduced in emacs 24. I think we > still support emacs 23 so this changeset adds two compatability > functions. Hi Mark; Can you give me a recipe to reproduce the failures in emacs23? I only get warnings when building. Of course I believe that things will go wrong at some point calling non-existent functions. d ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] emacs: add compatability functions for emacs 23
Some of the recent changes to the emacs code have used functions introduced in emacs 24. The functions used are read-char-choice and setq-local. This changeset adds compatability functions to notmuch-lib so that it should work on emacs 23. --- Hi I tried compiling under emacs 23 recently and noticed that some recent changes have used some features introuduced in emacs 24. I think we still support emacs 23 so this changeset adds two compatability functions. They are setq-local, which is esentially trivial, and read-char-choice. I have written a minimal version of read-char-choice for the functionality we need -- an alternative would be to just copy and paste the function from emacs 24 source. I have tested (lightly) on emacs 23 and it seems to work (and didn't before). It should not change anything on more recent emacs (i.e. in cases where we weren't already broken). Finally, it does leave a compiler warning when compiling under emacs 23: notmuch-company.el does not require notmuch-lib.el but I think that is probably OK. Best wishes Mark emacs/notmuch-lib.el | 18 ++ 1 file changed, 18 insertions(+) diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el index 1f0d167..1459f83 100644 --- a/emacs/notmuch-lib.el +++ b/emacs/notmuch-lib.el @@ -966,6 +966,24 @@ status." (defvar notmuch-show-process-crypto nil) (make-variable-buffer-local 'notmuch-show-process-crypto) +;; Compatability functions for emacs 23. + +(unless (fboundp 'setq-local) + (defmacro setq-local (var val) +`(set (make-local-variable ',var) ,val))) + +(unless (fboundp 'read-char-choice) + (defun read-char-choice (prompt chars) +(let (char done) + (while (not done) + (setq char (read-key prompt)) + (cond +((memq char chars) + (setq done t)) +((memq char '(?\C-g ?\e)) + (keyboard-quit + char))) + (provide 'notmuch-lib) ;; Local Variables: -- 2.1.4 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
