nicferrier / emacs-noflet

noflet - nic's overriding flet, for fleting functions for the purpose of decorating them
72 stars 17 forks source link

Support defining commands in noflet #17

Open DarwinAwardWinner opened 9 years ago

DarwinAwardWinner commented 9 years ago

Now if a function binding in "noflet" includes an interactive form, that function will be available as a command in BODY, (i.e. BODY can invoke it via "call-interactively").

Note that no corresponding feature is added for nolexflet.

Fixes #16.

DarwinAwardWinner commented 9 years ago

Actually, this code is broken in a few corner cases. Give me some time to fix it up before merging.

DarwinAwardWinner commented 9 years ago

Ok, I kind of nerd-sniped myself on this one and ended up rewriting the whole macro. I added two features. First, interactive functions (commands) are now supported. Second, there is a now shorthand for aliasing one function/command to another. Other than that, everything should keep working the same as before.

The new version uses cl-letf, which I suspect might be fairly new. I'm pretty sure it could easily be rewritten using cl-flet for more backward-compatibility.

DarwinAwardWinner commented 9 years ago

So this should fix #18 as well.

DarwinAwardWinner commented 9 years ago

Actually, on further testing, this code isn't working the way I expect. I'll have to play with it some more. (See next comment.)

DarwinAwardWinner commented 9 years ago

Ok, I realized why I was confused. It turns out that there's no need for a noflet* version where later function definitions can refer to previous ones, because since we're only redefining functions, nothing is actually evaluated until we get to body, at which point every function body can already refer to every other definition (and cannot refer to the original definitions of any overidden functions). So the only case where the order of redefinition matters is the case that I added: aliasing functions to each other. In particular, cases like swapping two functions will work with my code. So for example, this will not result in infinite recursion:

(noflet ((next-line previous-line)
         (previous-line next-line))
  (next-line))
  ;; Can also invoke it as a command
  (call-interactively 'next-line))
nicferrier commented 9 years ago

So we can close this, right?

DarwinAwardWinner commented 9 years ago

Well, do you want to merge it?

nicferrier commented 9 years ago

I'm no wildly enthusiastic about something that totally rewrites the whole thing. How do we know it doesn't break anything? it won't be you fixing it if it does. Maybe offer an alternative with your implementation?

DarwinAwardWinner commented 7 years ago

So, in answer to:

How do we know it doesn't break anything?

I finally got around to implementing a test suite. I also refactored my rewrite into a series of easy-to-digest commits each adding a single piece of functionality. To run the tests, you can check out my test-suite branch, load noflet.el and test/noflet-test.el, and then run M-x noflet-run-all-tests. If you have cask installed, you can also run the tests from the command line using cask install followed by cask exec ert-runner --no-win. The test-suite branch contains 3 short tests that all pass on the current version of noflet. My letf branch simply reimplements the current functionality of noflet using cl-letf, which significantly reduces the amount of code required in noflet|expand. This branch passes all tests in test-suite, but does not add any new functionality. Building on the letf branch, my interactive branch (the one in this pull request) implements the new functionality I originally proposed, namely fixing #16, adding the ability to alias existing functions by name, and fixing #18 (which is mostly only relevant when using the alias functionality). This branch also includes additional tests covering the new functionality, which all pass. (I need to add one more test for #18.)

So anyway, at a minimum, I'd recommend merging the test-suite branch and commit 640cbd63, which just fixes a docstring. If you're interested in the rest, then checkout the interactive branch, verify that all the tests pass for you, and merge that.

Lastly, if any of these are merged, the melpa recipe for noflet might need to be updated to only download the main elisp file and not the test files.