search

Bad-ptr / persp-mode.el

named perspectives(set of buffers/window configs) for emacs
400 stars 44 forks source link

Replace flet with cl-flet #75

Closed delaanthonio closed 6 years ago

delaanthonio commented 7 years ago

The macro flet is deprecated as of Emacs 24.3. Let's replace it with cl-flet.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 4.478% when pulling 8b18e2218d1e627051f363e46f2e5f88e60c0eb3 on beta1440:master into a33c91e3738996c058841ed1985c67bedd4875e1 on Bad-ptr:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 4.478% when pulling 8b18e2218d1e627051f363e46f2e5f88e60c0eb3 on beta1440:master into a33c91e3738996c058841ed1985c67bedd4875e1 on Bad-ptr:master.

Bad-ptr commented 7 years ago

You can't just replace flet(dynamic binding) with cl-flet(lexical binding) :). This will not work as expected.

I think it must be replaced with the letf. Try out develop branch, and if it solve the issue for you I will merge it into master. https://github.com/Bad-ptr/persp-mode.el/tree/develop e4257511315142024d7892a753ce14368666e838

delaanthonio commented 7 years ago

  1. You can't just replace flet(dynamic binding) with cl-flet(lexical binding) :). This will not work as expected.

Does replace flet with cl-flet always fail or does it fail in certain situations? I've tested the with-persp-buffer-list macro with cl-flet and it works fine for me. I get the following error when using letf or cl-left:

‘let’ bindings can have only one value-form: buffer-list, (&optional frame), (persp-buffer-list-restricted frame)

  1. Should I rebase this pull request on the develop branch?
Bad-ptr commented 7 years ago

Does replace flet with cl-flet always fail or does it fail in certain situations?

Well, actually I can not say for sure. I base my thoughts on the results of googling: https://stackoverflow.com/questions/18895605/should-flet-be-replaced-with-cl-flet-or-cl-letf https://www.reddit.com/r/emacs/comments/50n6cn/flet_compile_warning_on_emacs_25_start/ http://emacsredux.com/blog/2013/09/05/a-proper-replacement-for-flet/ From which I conclude that cl-flet is not the same as flet.

Another issue with the cl-flet is that it only available for modern Emacses which comes with cl-lib(hence the cl- prefix). But persp-mode uses older cl(without -lib suffix) library and it is not a good idea to mix them.

I get the following error when using letf or cl-left: ‘let’ bindings can have only one value-form: buffer-list, (&optional frame), (persp-buffer-list-restricted frame)

I guess it's because you misuse the letf form. letf ≠ flet. flet uses a function definition syntax(like defun). letf allows you to temporarily bind a place(the syntax is like setf): https://www.gnu.org/software/emacs/manual/html_node/elisp/Setting-Generalized-Variables.html

You can see the difference in the commit: https://github.com/Bad-ptr/persp-mode.el/commit/e4257511315142024d7892a753ce14368666e838

If with flet you would write the following:

(flet ((temp-function (arg) ...)) ...)

then the letf equivalent will be:

(letf (((symbol-function 'temp-function) #'(lambda (arg) ...))) ...)

Should I rebase this pull request on the develop branch?

Yes, if you are going to submit a pull request, do it against the develop branch.

However I think that my commit already fixes the problem, and it only waits for somebody who will test it (For me it seem to work). You can do it by just downloading the https://raw.githubusercontent.com/Bad-ptr/persp-mode.el/develop/persp-mode.el file and M-x package-install-file RET it.

If you can not test, let me know, I will merge it into master(I quite sure that it works:)) and we'll see how it will go.

Bad-ptr commented 6 years ago

Must be fixed in master. If not -- let me know.