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

Support defining commands in noflet #17

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

DarwinAwardWinner
Copy link
Contributor

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 DarwinAwardWinner force-pushed the interactive branch 2 times, most recently from e0d7394 to 56831f7 Compare April 16, 2015 19:46
@DarwinAwardWinner
Copy link
Contributor Author

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

@DarwinAwardWinner DarwinAwardWinner force-pushed the interactive branch 2 times, most recently from d2b71f3 to e26a26f Compare April 17, 2015 01:35
@DarwinAwardWinner
Copy link
Contributor Author

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
Copy link
Contributor Author

So this should fix #18 as well.

@DarwinAwardWinner
Copy link
Contributor Author

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
Copy link
Contributor Author

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

So we can close this, right?

@DarwinAwardWinner
Copy link
Contributor Author

Well, do you want to merge it?

@nicferrier
Copy link
Owner

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
Copy link
Contributor Author

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 640cbd6, 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.

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.

Bind commands?
2 participants