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

Fix module return value #91

Merged
merged 6 commits into from
Feb 6, 2022
Merged

Conversation

harrygallagher4
Copy link
Contributor

@harrygallagher4 harrygallagher4 commented Jan 25, 2022

Continuation of #90

  • added wrap-module-body macro
  • changed compile.macros-prefix to wrap body in wrap-module-body. That should probably be moved to its own function(?) or the name of macros-prefix should be changed
  • recompiled with these changes

This appears to generate the right lua for my dotfiles. It looks like parinfer changed the files more than necessary so I'll have to fix that tomorrow.

See:
https://github.com/harrygallagher4/nvim/blob/95ba7461baf53393bc4f6861d19565b1a940a098/fnl/dotfiles/init.fnl (module that has a return value other than *module*, now with *module* removed)
https://gist.github.com/harrygallagher4/847440c11921153d8f4fc6871bb49820#file-init-lua-L34-L38 (compiled lua)
https://github.com/harrygallagher4/nvim/blob/2cf6ff701aacb8a5cdf734216c2944c4f3f99804/fnl/dotfiles/test.fnl (non-aniseed module)
https://gist.github.com/harrygallagher4/847440c11921153d8f4fc6871bb49820#file-test-lua (compiled)

@harrygallagher4
Copy link
Contributor Author

harrygallagher4 commented Jan 27, 2022

Whoops, accidentally force-pushed without any changes which closed the PR 😅. Sorry if closing/reopening/commenting spams your email.

Anyway the latest commit removes the formatting changes introduced by parinfer. Unfortunately some tests are now failing. From here I'll need some input, if you like these changes then we should probably modify the failing tests. Also one of the tests complains about module not existing so it's possible this can be solved in a smarter way than my hacky macro. I don't have much experience with the fennel compiler so I'll read up and see if I come up with anything.

edit: speaking of that reading... the below commit uses (in-scope (sym "*module*")) instead of naively inserting *module* when it might be nil. As a bonus only one test and one assertion are failing now (and for a somewhat petty reason in my opinion)!

second edit: in-scope actually didn't work for this unfortunately

@Olical
Copy link
Owner

Olical commented Jan 31, 2022

Will look soon! Away visiting family + working remotely at the moment so will hopefully get time later in the week 😊 thank you for looking into this!

Now, rather than using (or *module* original-return), wrap-module-body
just wraps the last expression in yet another macro wrap-last-expr
which, when evaluated, has access to the scope as-of the last
expression, so we can test for *module* using (get-scope). This way if
*module* doesn't exist we can just leave everything untouched and if it
does, just append *module* so it gets returned.
@harrygallagher4
Copy link
Contributor Author

So, the latest commit has this at a state that I'm very happy with. Now I'm wrapping the body in a macro that wraps the last expression in another macro which can properly access the scope to check for *module*. I tested it on my dotfiles and everything is working fine, including one module that doesn't use (module ...) which outputs untouched lua. Also, no tests are failing! :)

❯ make test
SUFFIX="test/fnl/foo.fnl" scripts/test.sh
[aniseed.core-test] OK 36/36 tests and 123/123 assertions passed
[aniseed.eval-test] OK 3/3 tests and 36/36 assertions passedfoofoo
bar[aniseed.nvim.util-test] OK 2/2 tests and 7/7 assertions passed
[aniseed.string-test] OK 6/6 tests and 29/29 assertions passed
[aniseed.macros-test] OK 1/1 tests and 3/3 assertions passed
[aniseed.fs-test] OK 3/3 tests and 8/8 assertions passed
[aniseed.compile-test] OK 1/1 tests and 4/4 assertions passed
[total] OK 52/52 tests and 210/210 assertions passed

Also, I went ahead and implemented #66 for fun.
https://github.com/harrygallagher4/aniseed/tree/require-additional-macros

No rush on any of this, enjoy your time away :)

@Olical
Copy link
Owner

Olical commented Feb 6, 2022

I'm making some rename changes locally and reviewing, looking good! I've also wrapped Aniseeds internally bootstrapped files in the same macros so they get the same protection.

@Olical Olical merged commit d1b8ab9 into Olical:develop Feb 6, 2022
@Olical
Copy link
Owner

Olical commented Feb 6, 2022

Merged and compiled into Conjure's develop branch too! Let's give it a few days to soak and then I think it's probably good to go. So far it seems fine to me!

@Olical
Copy link
Owner

Olical commented Feb 6, 2022

I really like that it ensures some consistency across ALL files, no more slight chance of returning the wrong table.

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.

2 participants