Skip to content

Add devcontainer setup for Elm and fix Euler Elm example #911

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

Merged
merged 5 commits into from
Nov 7, 2021

Conversation

ShadowMitia
Copy link
Contributor

Add setup for Elm, part of #871

@ntindle
Copy link
Member

ntindle commented Nov 4, 2021

It seems to me the elm script I picked to test this is incompatible with the current version of elm. Not sure why they would change the whole process but did you get any elm to run with the binaries that are installed?

Also the binaries work fine but it feels weird to merge if they can't run our elm.

@jiegillet
Copy link
Member

The Elm contribution is mine, it's written in Elm 0.18, since then 0.19.1 came out, and that's the version of the binary.
I'm willing to update the Elm code to 0.19.1. Would that solve the issue?

@ShadowMitia
Copy link
Contributor Author

I don't know anything about Elm, I think the Elm script didn't work on my end either, but I figured it installed properly so it was good enough.

Has Elm changed that much between versions?

@ntindle
Copy link
Member

ntindle commented Nov 4, 2021

The Elm contribution is mine, it's written in Elm 0.18, since then 0.19.1 came out, and that's the version of the binary.

I'm willing to update the Elm code to 0.19.1. Would that solve the issue?

I believe that would solve the issue. Thanks for being willing to update

@jiegillet
Copy link
Member

Has Elm changed that much between versions?

Yeah, there were a bunch of breaking changes from 0.18 to 0.19, but 0.19 is supposed to be stable.

Elm is a frontend development language, so it's not something you can run as is like a script. You can use the elm executable to create an .html or .js file, or use it to run a local server for development.

@jiegillet
Copy link
Member

jiegillet commented Nov 4, 2021

Is it cool to push to this branch? I'm afraid very few people know Elm, so waiting for a review would take ages. However, this is just an update, I had to change to different packages, etc, but the algorithm itself wasn't touched at all.

If you run elm make src/Euler.elm from the elm directory, you should get a index.html that you can open in the browser. If that works, it's all good.

@ShadowMitia ShadowMitia changed the title Add devcontainer setup for Elm Add devcontainer setup for Elm and fix Elm example Nov 4, 2021
@ShadowMitia ShadowMitia changed the title Add devcontainer setup for Elm and fix Elm example Add devcontainer setup for Elm and fix Euler Elm example Nov 4, 2021
@ntindle
Copy link
Member

ntindle commented Nov 4, 2021

Yes it's okay. I'll review later today

@ntindle
Copy link
Member

ntindle commented Nov 5, 2021

Okay great news! This certainly runs. I have no clue how to review if it "works" from an algo perspective. Here is the output. @leios or someone else with good math skills please run this (after unzipping) wiht something like

python3 -m http.server 9929
to serve it and verify its functional.
index.zip

The dev container works though.

@ntindle ntindle self-requested a review November 5, 2021 19:53
@ShadowMitia
Copy link
Contributor Author

@jiegillet I found a bug where when you press start and then during the animation press start again, the animation runs, but it doesn't correspond to the original animation. Is this a bug in the migration? I couldn't make the code run on an older version of elm...

@jiegillet
Copy link
Member

Haha whoops, that was just me being sloppy, 0.18 or 0.19. I fixed it :)

@ntindle ntindle merged commit a3825b2 into algorithm-archivists:master Nov 7, 2021
@ntindle ntindle mentioned this pull request Nov 16, 2021
20 tasks
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.

3 participants