exercism / elm-analyzer

GNU Affero General Public License v3.0
1 stars 4 forks source link

Add docker integration, CI, and internal `elm-review` #27

Closed jiegillet closed 2 years ago

jiegillet commented 2 years ago

This PR is meant to address everything left to do before deployment.

Docker

It took me a while to figure out how docker works and how to debug it (especially since both elm and elm-review don't have explicit offline modes), but thanks to useful advice from Slack, I think I managed to get to something that works. I ended up with something very similar to elm-test-runner. Of course, please do review carefully since this is my first docker build.

I made various changes to the shell scripts so that the same ones can run both locally and in docker.

CI

I copied docker.yml pretty much as is (I only removed the master branch) from an Elixir repo, that should be fine. It doesn't run on my fork, probably @ErikSchierboom has to do some admin magic on it?

elm_tests.yml runs all the tests with elm-test (not elm-test-rs because version 1.1 doesn't allow tests with the same name, but IMO they make sense here. I didn't check later versions), checks the formatting, runs elm-review on the repo, and runs run-tests-in-docker.sh, which is basically bin/smoke_test.sh in the container.

elm-review

I took the approach of (quote from README)

As a rule of thumb, if we expect the students to follow specific rules when writing their solutions, we should hold ourselves to the same standards in our source code.

So I would like to keep the rules from the repo the same as the common rules we use for the students (we don't have any active at the moment, but I'm expecting we will add NoUnused and Simplify at some point).

All changes from the elm code was suggested from those rules.

README

I wrote one.

Going live

We will need to merge that PR so we can check if the analyzer works as intended. You will get a comment if you use either List.filter or List.filterMap.

Maintaining the repo

At this point, I would like to request to become a maintainer on the Elm track, if I can get the blessing of @mpizenberg and @ceddlyburge.

I changed the CODEOWNER file so that we don't need admins to review each PR that don't touch the .github repo.

ceddlyburge commented 2 years ago

At this point, I would like to request to become a maintainer on the Elm track, if I can get the blessing of @mpizenberg and @ceddlyburge.

You have my blessing :)

mpizenberg commented 2 years ago

Mine too :)

jiegillet commented 2 years ago

I know I was told not to worry, but I modified run.sh and the smoke test so that it would run in read-only (but still writing in /tmp).

Is there anything left to do before generating the secrets and activating the analyzer in the Elm config file?

mpizenberg commented 2 years ago

I know I was told not to worry, but I modified run.sh and the smoke test so that it would run in read-only

Haha It's like you just climbed a mountain, you have a great view, but there is this last spot, 50m away, and your journey would not be complete until you make sure the view isn't better from up there.

If I have a small nitpick (without checking in detail all the PR), why are some files are with an eol at the eof while some do not have an eol? I think the posix standard ask that we add an eol as the last char of a file. If you use VSCode it should be this setting: "files.insertFinalNewline": true

jiegillet commented 2 years ago

Haha yeah, it's that kind of feeling. And when you reach this other perfect peak, you spot another one :)

Thank you for the EOL tip, it is something that I never really paid attention to, I do use VSCode and I've enabled the setting.

ErikSchierboom commented 2 years ago

From my point of view, this is ready to be merged!

ceddlyburge commented 2 years ago

me too :)