search

ahyatt / emacs-websocket

A websocket implementation in elisp, for emacs.
GNU General Public License v2.0
321 stars 42 forks source link

websocket-test.el: ‘flet’ is an obsolete macro (as of [emacs] 24.3) #71

Closed sten0 closed 4 years ago

sten0 commented 4 years ago

Hi,

Prioritise this however you'd like. I'm only reporting it because someday the obsolete macro may be removed.

websocket-test.el: ‘flet’ is an obsolete macro (as of 24.3); use either ‘cl-flet’ or ‘cl-letf’; use either ‘cl-flet’ or ‘cl-letf’. It seems like it work be orthogonal to converting websocket-functional-test.el and websocket-test.el to also use lexical-binding.

P.S. I'm also curious why you're using explicit dynamic binding by using defvar instead of setq. To be fair, my bias is "use lexical, whenever possible" ;-)

Thanks, Nicholas

ahyatt commented 4 years ago

Thanks for reminding me, I fixed this just now, in commit 341ec2b9d6782c61d32256bd29fe6c751ed7690f.

As to why I use dynamic scope with letf in the first place, it's useful for unit tests. I'm not sure which defvar you are mentioning, though.

sten0 commented 4 years ago

Thanks for reminding me, I fixed this just now, in commit 341ec2b9d6782c61d32256bd29fe6c751ed7690f.

Thanks!

As to why I use dynamic scope with letf in the first place, it's useful for unit tests. I'm not sure which defvar you are mentioning, though.

That makes sense, and I have to confess I'm not familiar with the unit test-specific pros and cons of dynamic scope (other than the nullprogram.com .

Most of them, but of course everyone is entitled to use to their own style :-) The reasons for my concern are: 1. I've noticed a higher than average incidence of subtle and/or bizarre and indeterminate CI failures in packages that continue to prefer dynamic binding by default 2. By the time I started to get serious about elisp, lexical scope was the strongly recommended best practise...and because of this I have to confess potential bias.

At this point I'm not sure if #72 is a normal bug or one of those tricky (1) bugs.

Cheers! Nicholas

sten0 commented 4 years ago

P.S. There may also be upstream pressure to transition https://git.savannah.gnu.org/cgit/emacs.git/tree/etc/NEWS?h=emacs-27.1#n326 and I suspect that might be the cause of the CI regressions I've noted in various packages that use dynamic binding. We'll soon switch to Emacs 27.1 in Debian, but hopefully my worries are unfounded ;-)