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

[RDY] Drop moonscript #1128

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@tarruda
Copy link
Member

tarruda commented Aug 30, 2014

As described by @fwalch in #1098, moonscript is not well-supported by busted, which was causing bugs when updating busted to the newest version. To avoid this and related bugs, moonscript is completely dropped in this PR. The existing moonscript code has been compiled to lua with some manual tweaks to improve readability

This kind of grunt work is a good exercise to macro skills :)

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Aug 30, 2014

👍

Although it wouldn't have been necessary for this PR, I'll mention this for other refactors:

https://github.com/facebook/codemod

@tarruda tarruda force-pushed the tarruda:drop-moonscript branch from a18c62f to a6a7d81 Aug 31, 2014

@tarruda tarruda changed the title [WIP] Drop moonscript [RDY] Drop moonscript Aug 31, 2014

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 31, 2014

@stefan991 your comment seems to have disappeared, could you post a patch with the cosmetic changes you mentioned?

@stefan991

This comment has been minimized.

Copy link
Contributor

stefan991 commented Aug 31, 2014

This would be a good opportunity to fix the test structure: Fileinfo is inside folder operations, but it should be one level up. See saimen@8d29104 from #855

@tarruda patch: https://gist.github.com/stefan991/ebb2cd525d99c7d212e0

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 31, 2014

Done with this PR, @aktau / @justinmk feel free to merge anytime

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 31, 2014

.deps/usr/bin/busted_bootstrap --lpath="./build/?.lua" test

Doing it now.

By the way, I find that since recently I have to run the tests like this:

.deps/usr/bin/busted_bootstrap --lpath="./build/?.lua" test

Instead of the other alternative that we recently had to change to:

.deps/usr/bin/luajit -- .deps/usr/bin/busted_bootstrap --lpath="./build/?.lua" test

Yet make unittest still doesn't work:

luajit: ...evin/github/neovim/neovim/.deps/usr/bin/busted_bootstrap:3: unexpected symbol near '-'
CMake Error at /Users/Kevin/github/neovim/neovim/cmake/RunUnittests.cmake:13 (message):
Unit tests failed.

Not that I think it's related to this PR. But, did I miss something?

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 31, 2014

👍 merged, thanks

@aktau aktau closed this Aug 31, 2014

aktau added a commit that referenced this pull request Aug 31, 2014

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 31, 2014

By the way, I find that since recently I have to run the tests like this:

yes, this seems to be an incompatibility bug with the new luarocks version and should be fixed by #1098.

Another temporary workaround(that I have used in neovim/deps) is to remove busted and symlink busted_bootstrap to busted

@tarruda tarruda referenced this pull request Sep 7, 2014

Merged

September Newsletter #71

@roryokane

This comment has been minimized.

Copy link

roryokane commented Sep 27, 2014

People who are interested in the reasoning behind this decision can find more information in the “Dropping of MoonScript” section of the September newsletter and in this comment about that section.

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