Ruby.2

[cookrn]

I'm not sure if you consider accepting additional implementations for already completed languages, but I recently finished one in Ruby that passes all the tests. I definitely couldn't have figured it all out without reviewing some of the tricky details of the current implementation! It was very fun to work through :sunglasses:

A few aspects of this implementation as compares to the included one in Ruby:

• No global variables
• 2 space tabs
• No modification (monkey-patching) of core ruby classes
• All code contained within Mal namespace

This certainly isn't a criticism, but these are some techniques I used to try to make the implementation more idiomatic if one were to compare to other Ruby code in the wild. If you're open to it, I'd love to polish this more to help get it merged. Cheers!

[kanaka]

I'm willing to merge an additional ruby implementation (especially since it's fairly different). This was basically my second program using ruby so I'm sure it's not particularly idiomatic. One of the things I really enjoyed about ruby is that it seemed very "Lispy" under the hood so I tried to leverage that as much as possible. The ruby implementation is one of the smaller implementations as a result.

I think my only comment about the code itself is that core.rb seems fairly verbose. I think some of that comes from the fact that you have user-defined types for symbols, true, false, and nil. Wouldn't it still be fairly idiomatic to use ruby's built-ins for that? Not a blocker for merging, it just stood out to me as really large compared to my implementation.

For me to merge it, you'll need to get the CI stuff working. You'll need a stub Dockerfile and Makefile. The following commands need to work:

make "docker-build^ruby.2"
make DOCKERIZE=1 "test^ruby.2"

[cookrn]

Great! I'll work on getting the CI build to pass.

Regarding the distinct types for true, false, nil, and symbols, I found that as I was working through the steps it was helpful to know in an obvious and visible way when I was using the types for something related to Mal interpretation separate from internal Ruby logic. Also, maybe an optimization too far, but it seemed like it could be helpful in a running system using Ruby and Mal w/ interop to know which data was coming from which language.

Anyways, I'm very open to changing to using the Ruby types for those value explicitly if it would be better for all current and future Mal users, so just let me know if it's important to change that in order to be merged :smiley:. A different way to improve the readability of core.rb could be to divide it into more files in a core/ directory based on the function intent.

[cookrn]

@kanaka is each implementation pushed as its own image to Dockerhub prior to running in CI? Apologies, I don't quite understand how to get the Docker image to be accessible during the build.

[kanaka]

Yes, the CI pulls a docker image for each implementation of the form: kanaka/mal-test-IMPL. And for now I have to build and push those images before the CI for that implementation will run I have to build and push an image. At some point I might add another variable to IMPLS.yml that overrides the docker image repo path in which case the implementors themselves could build and push an image to their own docker hub account. Alternatively, I might implement a fall back that would do a full docker build of the image if the pull fails. However, that's MUCH slower and CPU intensive than pulling the image so I wouldn't want that to be default. Anyways, still something I'm mulling over.

In the meantime, I've built and pushed a kanaka/mal-test-ruby.2 image. Please make the following changes and update the branch. The BUILD and MAL_IMPL variables are related to self-hosting testing and not multiple implementation testing. Also, I don't want versions of the docker images to arbitrarily float forward so I use fixed image versions. I suspect the CI will pass once you do this:

diff --git a/IMPLS.yml b/IMPLS.yml
index edb9c6aa..38f8ad77 100644
--- a/IMPLS.yml
+++ b/IMPLS.yml
@@ -81,7 +81,7 @@ IMPL:
   - {IMPL: rexx}
   - {IMPL: rpython, SLOW: 1}
   - {IMPL: ruby}
-  - {IMPL: ruby.2, MAL_IMPL: ruby, BUILD_IMPL: ruby}
+  - {IMPL: ruby.2}
   - {IMPL: rust}
   - {IMPL: scala}
   - {IMPL: scheme, scheme_MODE: chibi}
diff --git a/impls/ruby.2/Dockerfile b/impls/ruby.2/Dockerfile
index 486526eb..79d6c03f 100644
--- a/impls/ruby.2/Dockerfile
+++ b/impls/ruby.2/Dockerfile
@@ -1,4 +1,4 @@
-FROM ubuntu:latest
+FROM ubuntu:21.04
 MAINTAINER Joel Martin <github@martintribe.org>

 ##########################################################

[kanaka]

@cookrn I forgot to actually pull the trigger on pushing out the docker image. I've done that now and restarted the CI.

[cookrn]

@kanaka no problem, looks like it passed :fireworks:

Do you know whether it's expected that the other builds are failing?

[kanaka]

Okay, tests look good. The elm and wasm failures can be ignored. The only thing left is to add your implementation to the README with a brief description of the differences from the first ruby implementation and then bump the implementations and runtime modes count values at the top of the README. Once you do that I'll merge it.

[kanaka]

And don't forget to fix the FROM line of your Dockerfile to use ubuntu:21.04 (otherwise the next time I rebuild and push, it will unnecessarily bump the version and might cause subtle issues that are tough to track down).

[cookrn]

@kanaka I just realized I forgot to complete the interop. Would it be best to finish that now before merging? Or update later if/when that's complete?

README change should be complete now.

[kanaka]

Interop is completely optional so I went ahead and merged it. You can add a new PR when you have interop ready.

Congrats and thanks!

[cookrn]

Awesome, thanks for merging!