mal implementation in Elm 0.18 ported to Elm 0.19 #450
Conversation
|
@Pancakem did you intend to close this? Seems like there are some good improvements here. Perhaps you just closed it to fix the build/Travis issue? |
Yes, I closed it to fix issues. |
|
Hi @Pancakem, Is this ready for merging? The Travis tests won't pass until the upstream docker hub image is updated. If it's passing for you locally, then I'll update the image so that Travis will pass too. |
|
@Pancakem sorry for the slow reply. I just had a chance to look at this. I forgot that elm uses npm for installation so it doesn't actually require me to update the Docker image upstream. Looks like their are a few macro failures and the perf3 micro-benchmark which is timing out: The build is at: https://travis-ci.org/github/kanaka/mal/jobs/684724080. Lines 1293-1314 show the macro failures and the timing out test is at line 3321 Once those issues are fixed then I will merge it. |
Thank you. I will fix that. |
|
Hi @Pancakem I know it's been a long time but I was wondering if you've had any chance to look at this. The current elm implementation of mal in elm (0.18) has been broken for a while because the elm-0.18 Linux package was removed quite a while ago. If I can't find somebody to resurrect the elm build/impl, then I'll probably need to remove it from mal. @c0deaddict since you are the original creator of the elm mal implementation, any interest in getting it working again with elm 0.19 if @Pancakem isn't able to? |
|
I can have a look at it, if @Pancakem doesn't have the time. That probably won't be sooner than February next year, a busy period is coming up and I'm currently focusing on getting the Advent of Code completed 😄 |
It was necessary to rename some ambiguous variables. Some more names could probably be changed in order to reduce the diff with kanaka#450 (my names were choosen in order to reduce the diff with master...) Peek ideas from kanaka#450: - sort imports - skip a line between '->' or before 'else' - no indentation after 'in' - fix indentation when it was only intended to reduce diff - remove some unneeded parenthesis and - if .. return True else False -> ...
|
Hello @Pancakem and @c0deaddict. |
It was necessary to rename some ambiguous variables. Some more names could probably be changed in order to reduce the diff with kanaka#450 (my names were choosen in order to reduce the diff with master...) Peek ideas from kanaka#450: - sort imports - skip a line between '->' or before 'else' - no indentation after 'in' - fix indentation when it was only intended to reduce diff - remove some unneeded parenthesis and - if .. return True else False -> ...
Various trivial changes reducing the diff to kanaka#450. Dockerfile: npm already depends on nodejs Core.elm: change profile of deepEquals instead of uncurrying before each call.
|
Nice work @asarhaddon ! I glanced over your changes and they look good to me. I do have to admit that I haven't read or written Elm code for a few years, so i'm a bit rusty. I agree with you that the best way forward is to get your PR merged. The styling issues can be fixed later on, as a further improvement. |
Integrates 0.19.0 updates done at: kanaka/mal#450 with updates to 0.19.1 such as updating to 0.19.1 (with elm directly installed) and some Makefile updates to make things build. This still has a couple of runtime issues: - macros that are defined using a quote or quasiquote at the top-level aren't evaluated the final time. - the perf3 test times out (this may be related to the macros issue).
No description provided.