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

Implement loader for Fennel #740

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HiPhish
Copy link

@HiPhish HiPhish commented Mar 9, 2024

Closes #738

Tests can be written in Fennel if fennel is among the loaders. Currently the traceback is wrong for failures and pending tests, but it is wrong for tests written in Moonscript as well, so this seems to be a general problem with busted.

It would have been cool if we could make it, describe and so on into macros to not have to manually wrap the test body inside a thunk every time.

;; With macro
(it "Adds two numbers"
  (assert.are.equal 5 (+ 2 3)))

;; Without macro
(it "Adds two numbers"
  (fn []
    (assert.are.equal 5 (+ 2 3))))

This macro can accomplish it when added to the beginning of a test:

(macro it [description & body]
  `(let [fenv# (or (and getfenv (getfenv)) _ENV)]
     ((. fenv# :it) ,description
                (fn [] ,(unpack body)))))

However, the source map produces rather weird mappings from Lua to Fennel (it maps the assertion to the body of the macro definition), so for now I won't pursue this.

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first blush this looks like it's going to be fine. I don't have any Fennel expertise but it isn't like this is going to break anything else, and I don't expect any real reason to not include such a module if Fennel folks say it is useful.

I'll review details later this week (feel free to poke again if this falls off my radar), but to quick things:

  1. I've been roughly following the conventional commit spec, so this commit message should be feat(core): Implement loader for Fennel or something like that.
  2. We should probably add a test. Somehow...obviously we're going to have a dependency problem with that but we can fix that in CI and maybe make it an optional test for local testing for folks that don't have Fennel setup. Is their prior art for testing our other loaders?

@HiPhish
Copy link
Author

HiPhish commented Mar 10, 2024

Thank you for your response. I have amended the commit message.

We should probably add a test.

Maybe I am missing something here, but it does not look like there are any loader tests. There is the file spec/modules/file_loader_spec.lua, but all it does is require and call a module, but it does not do anything with the results.

and maybe make it an optional test for local testing for folks that don't have Fennel setup.

That's simple enough, we can pcall(require, 'fennel') in the test; if the call errors then Fennel is not available and we can skip the Fennel loader.

I don't have any Fennel expertise

Fennel is pretty much just Lua with Lisp notation (some special forms plus a macro system), so it maps very closely to Lua.

Tests can be written in Fennel if `fennel` is among the loaders.
Currently the traceback is wrong for failures and pending tests, but it
is wrong for tests written in Moonscript as well, so this seems to be a
general problem with busted.
@HiPhish
Copy link
Author

HiPhish commented Mar 11, 2024

I just pushed an update that does two things:

  • Wrap Fennel's compileString inside a pcall in case there is a syntax error in the Fennel file
  • Pass through compilation errors so they get reported properly
  • Delay registering the source map until the call to loadfile_ has actually succeeded

Previously the code was working under the assumption that compilation would always succeed.

@HiPhish
Copy link
Author

HiPhish commented Mar 18, 2024

feel free to poke again if this falls off my radar

poke @alerque

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Fennel loader?
2 participants