Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace flet with cl-flet #75

Closed
wants to merge 1 commit into from
Closed

Conversation

delaanthonio
Copy link

@delaanthonio delaanthonio commented Aug 2, 2017

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

Flet is a deprecated macro as of Emacs 24.3.
@coveralls
Copy link

coveralls commented Aug 2, 2017

Coverage Status

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

1 similar comment
@coveralls
Copy link

Coverage Status

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

@Bad-ptr
Copy link
Owner

Bad-ptr commented Aug 12, 2017

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
e425751

@delaanthonio
Copy link
Author

  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
Copy link
Owner

Bad-ptr commented Aug 19, 2017

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. letfflet.
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: e425751

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
Copy link
Owner

Bad-ptr commented Sep 4, 2017

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

@Bad-ptr Bad-ptr closed this Sep 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants