[#722] New practice exercise `yacht` #760

elibdev · 2021-06-13T05:37:59Z

This is my first contribution to this project, so I'm not sure if I've done everything right. I haven't edited the track config because I'm not sure how the difficulty would be rated. I would be happy to make any necessary changes to this PR. Thanks!

github-actions · 2021-06-13T05:38:11Z

Thank you for contributing to exercism/elixir 💜 🎉. This is an automated PR comment 🤖 for the maintainers of this repository that helps with the PR review process. You can safely ignore it and wait for a maintainer to review your changes.

Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:

General steps
- 🏆 Does this PR need to receive a label with a reputation modifier (reputation/contributed_code/{minor,major})? (A medium reputation amount is awarded by default, see docs)
Any exercise changed
- 👤 Does the author of the PR need to be added as an author or contributor in <exercise>/.meta/config.json (see docs)?
- 🔬 Do the analyzer and the analyzer comments exist for this exercise? Do they need to be changed?
- 📜 Does the design file (<exercise>/.meta/design.md) need to be updated to document new implementation decisions?
Practice exercise changed
- 🌲 Do prerequisites, practices, and difficulty in config.json need to be updated?
- 🧑‍🏫 Are the changes in accordance with the community-wide problem specifiations?
Practice exercise tests changed
- ⚪️ Are all tests except the first one skipped?
- 📜 Does <exercise>/.meta/tests.toml need updating?

Automated comment created by PR Commenter 🤖.

neenjaw · 2021-06-13T07:45:43Z

exercises/practice/yacht/README.md

@@ -0,0 +1,21 @@
+# Yacht


This file can be deleted

neenjaw · 2021-06-13T07:46:45Z

exercises/practice/yacht/mix.exs

+    [
+      app: :yacht,
+      version: "0.1.0",
+      # elixir: "~> 1.12",


While we encourage students to write using 1.12, our solution needs to be compatible back to 1.8 for the time being

Suggested change

# elixir: "~> 1.12",

# elixir: "~> 1.8",

Got it, I'll edit this and make sure it works with 1.8. Thanks!

neenjaw · 2021-06-13T07:58:34Z

exercises/practice/yacht/test/yacht_test.exs

+defmodule YachtTest do
+  use ExUnit.Case
+
+  @data Application.app_dir(:yacht, "priv/canonical-data.json")
+        |> File.read!()
+        |> Jason.decode!()
+
+  for test_case <- @data["cases"] do
+    test test_case["description"] do
+      assert Yacht.score(
+               unquote(test_case["input"]["category"]),
+               unquote(test_case["input"]["dice"])
+             ) == unquote(test_case["expected"])
+    end
+  end
+end


I think this is pretty cool, but this is incompatible with the v3 test runner at present because it would require jason to be available and as it is presently we don't have that build capacity. I don't want you to throw this away though, as I think there might be an application for this approach at some point.

In the mean time can you create a test suite file similar to how the other practice exercises are laid out?

It's really awesome that you attempted to write a test generator! It is something that we would want to have in the track at some point, but it's a really big project that requires good planing beforehand and knowledge of how the track and the platform works. If you are interested in doing that, we should open a GitHub issue and talk about the requirements. There was a previous attempt that was abandoned (https://github.com/exercism/elixir/pull/521/files).

One of the requirements would be that the test generation needs to happen beforehand, not on the student's computer. The student needs to receive a ready test file with all of the test cases. The test file, in most exercises, is the only way to learn the requirements for the solution. The student shouldn't need to read canonical-data.json to know how to solve the exercise. The student ideally should not even see the canonical-data.json file 🙂. We want to encourage TDD, so the student also needs to be able to un-skip every test case one by one, and that's why all test files have pending tags for all tests but the first one.

Thanks for explaining the context around this and why this isn't a valid approach for the way the exercises work. I will definitely update this PR to include a test suite with the tests all explicitly written out so it will work as expected for students. I am interested in helping with that test file generation project, but I understand how it's separate and outside the scope of the exercises.

neenjaw · 2021-06-13T08:02:54Z

exercises/practice/yacht/mix.exs

+  defp deps do
+    [
+      {:jason, "~> 1.2"}
+    ]
+  end


We can't use external dependencies in the test runner presently. See note down below about this

Not only the test runner cannot handle external dependencies, we also have no process of explaining to students how external dependencies work and how to use them. If we were to allow external dependencies in exercises, we would need to edit a lot of student documentation on how to run the tests (e.g. mention that some exercises need mix install before doing them) because many students are completely new to Elixir. It would also add a big problem of dependency version updates. We want to keep a broad range of Elixir versions in which the exercises can be solved, and sometimes it might not even be possible to have a single dependency version that runs on all Elixirs from 1.8 to 1.12. For all those reasons, we made a decision not to use any external dependencies for code that the student needs to run.

I think it's our fault that we didn't communicate this properly beforehand. This should be mentioned in CONTRIBUTING.md 🤔. I'll open a PR.

PS: let's not forget to remove the mix.lock file from this PR 🙂

No problem, I will update the PR without the dependency. I mostly did this as a fun exercise for myself so please don't worry about it not being documented explicitly (the contributing documentation is very helpful). I had a feeling it might not fit in the project as-is but I'm happy to make the changes so it does.

neenjaw · 2021-06-13T08:46:13Z

exercises/practice/yacht/.meta/gen_tests_toml.exs

@@ -0,0 +1,48 @@
+# Generate tests.toml file from canonical-data.json


Interesting approach here, we haven't done anything like this yet to generate test files or test toml configurations so I think this might not work with our current set up for v3. Let me take a think what might need to change to accommodate this method.

I learned 2 days ago that Exercism already has a tool that generates tests.toml files. It's a feature of configlet. @elibdev it's another failure on my part that I didn't mention this anywhere in the ticket or in the track's documentation. I'm going to open a PR that explains toml files and how to generate them.

Great work on your part for automating this 👍 but this script can be deleted now 🙂

Great, I'll delete this. Thanks!

angelikatyborska

Thank you a lot for the contribution 🎉. I am really sorry that I didn't write exhaustive documentation right away, which caused you to try very reasonable and clever automated approaches that need to be undone. I hope you have the time and motivation to do the changes, we would love to merge this PR with a new exercise 💜

I haven't edited the track config because I'm not sure how the difficulty would be rated.

Judging by your example solution, I would say... a 5? It's just a guess, it doesn't have to be perfect 😬

For "prerequisites", I would suggest:

"lists",
"cond",
"case",
"if",
"multiple-clause-functions",
"pattern-matching",
"guards",
"enum"

And for "practices", I would suggest "enum", but you would also need to remove "enum" from "practices" of another exercise, for example... Zebra Puzzle? There is a limit that a concept can be practiced by maximum 10 exercises.

angelikatyborska · 2021-06-13T09:44:36Z

exercises/practice/yacht/mix.exs

+  defp deps do
+    [
+      {:jason, "~> 1.2"}
+    ]
+  end


Not only the test runner cannot handle external dependencies, we also have no process of explaining to students how external dependencies work and how to use them. If we were to allow external dependencies in exercises, we would need to edit a lot of student documentation on how to run the tests (e.g. mention that some exercises need mix install before doing them) because many students are completely new to Elixir. It would also add a big problem of dependency version updates. We want to keep a broad range of Elixir versions in which the exercises can be solved, and sometimes it might not even be possible to have a single dependency version that runs on all Elixirs from 1.8 to 1.12. For all those reasons, we made a decision not to use any external dependencies for code that the student needs to run.

I think it's our fault that we didn't communicate this properly beforehand. This should be mentioned in CONTRIBUTING.md 🤔. I'll open a PR.

PS: let's not forget to remove the mix.lock file from this PR 🙂

angelikatyborska · 2021-06-13T09:53:07Z

exercises/practice/yacht/test/yacht_test.exs

+defmodule YachtTest do
+  use ExUnit.Case
+
+  @data Application.app_dir(:yacht, "priv/canonical-data.json")
+        |> File.read!()
+        |> Jason.decode!()
+
+  for test_case <- @data["cases"] do
+    test test_case["description"] do
+      assert Yacht.score(
+               unquote(test_case["input"]["category"]),
+               unquote(test_case["input"]["dice"])
+             ) == unquote(test_case["expected"])
+    end
+  end
+end


It's really awesome that you attempted to write a test generator! It is something that we would want to have in the track at some point, but it's a really big project that requires good planing beforehand and knowledge of how the track and the platform works. If you are interested in doing that, we should open a GitHub issue and talk about the requirements. There was a previous attempt that was abandoned (https://github.com/exercism/elixir/pull/521/files).

One of the requirements would be that the test generation needs to happen beforehand, not on the student's computer. The student needs to receive a ready test file with all of the test cases. The test file, in most exercises, is the only way to learn the requirements for the solution. The student shouldn't need to read canonical-data.json to know how to solve the exercise. The student ideally should not even see the canonical-data.json file 🙂. We want to encourage TDD, so the student also needs to be able to un-skip every test case one by one, and that's why all test files have pending tags for all tests but the first one.

angelikatyborska · 2021-06-13T09:57:19Z

exercises/practice/yacht/.meta/gen_tests_toml.exs

@@ -0,0 +1,48 @@
+# Generate tests.toml file from canonical-data.json


I learned 2 days ago that Exercism already has a tool that generates tests.toml files. It's a feature of configlet. @elibdev it's another failure on my part that I didn't mention this anywhere in the ticket or in the track's documentation. I'm going to open a PR that explains toml files and how to generate them.

Great work on your part for automating this 👍 but this script can be deleted now 🙂

angelikatyborska · 2021-06-13T10:03:35Z

exercises/practice/yacht/.meta/example.ex

+
+  def score("full house", dice) do
+    full_house =
+      Enum.frequencies(dice)


This function is only available from Elixir 1.10. One of the challenges of the example solution is that it needs to work in older versions too, currently we start testing from 1.7. I'm going to update the documentation to make this more explicit.

We had this problem with another exercise already 🙂 here:

elixir/exercises/practice/word-count/.meta/example.ex

Lines 27 to 33 in 0caae7f

defp frequencies(strings) do

# Enum.frequencies is only available from Elixir 1.10

strings

|> Enum.reduce(%{}, fn word, count ->

Map.update(count, word, 1, &(&1 + 1))

end)

end

Got it, I'll update my solution to not use Enum.frequencies/1 and will test it on Elixir 1.7 to make sure it works with that as well.

elibdev · 2021-06-13T16:20:00Z

Thanks for all the feedback! I'll make the necessary changes that you've helpfully laid out. For the track config "practices" would it be better if I changed my solution to not use the Enum module as much and use for instead, if there are too many exercises that require Enum?

angelikatyborska · 2021-06-13T16:50:52Z

For the track config "practices" would it be better if I changed my solution to not use the Enum module as much and use for instead, if there are too many exercises that require Enum?

No, don't worry about it! I think it's the nature of Exercism exrcises and Elixir's standard library that Enum plays a big role in almost all of them 😉. As far as I know, only a small subset of operations on enumerables is easier to achieve with for than with the Enum module (e.g. creating cartesian products). For this exercise, Enum is better suited, so let's keep it.

…r 1.7

elibdev · 2021-06-15T02:24:43Z

A couple things I wasn't sure about:

In the track config, I just generated a new UUID v4 fo the exercise. Is that exercise UUID supposed to come from somewhere instead?
I put the "practices" as "multiple-clause-matching" so we don't have to change an existing Enum exercise. I think for this exercise the multiple clause matching does apply but I'm not sure if that's the right one to pick.

angelikatyborska

Excellent ❤️ I left a small (optional) suggestion. The configlet / lint is failing due to a typo in a filename, nothing serious.

Is that exercise UUID supposed to come from somewhere instead?

No, generating a new unique value is correct 👍

I think for this exercise the multiple clause matching does apply but I'm not sure if that's the right one to pick.

Sounds like a good pick to me! It's really hard to tell at the beginning. For existing exercises, we analyzed some community solutions to see which approaches people use the most. Here, we only have one solution... but I feel good about suggesting to students that they should solve this one with multiple clause functions 😄

angelikatyborska · 2021-06-15T06:50:08Z

exercises/practice/yacht/test/yach_test.exs

@@ -0,0 +1,143 @@
+defmodule YachtTest do


There's a typo in the file name

exercises/practice/yacht/lib/yacht.ex

Co-authored-by: Angelika Tyborska <[email protected]>

angelikatyborska · 2021-06-15T11:55:18Z

All green = merge 🚀

Thank you again 💜. The next exercise won't be available on exercism.io until v3 is launched, but you will be able to check it out on v3 staging (https://exercism.lol/tracks/elixir).

If you enjoyed adding a new exercise, I added a few more issues about more practice exercises 😇

elibdev · 2021-06-15T16:35:57Z

Thank you for all your help, and I will check out the new exercise issues! 💜

neenjaw self-requested a review June 13, 2021 07:43

neenjaw reviewed Jun 13, 2021

View reviewed changes

angelikatyborska reviewed Jun 13, 2021

View reviewed changes

angelikatyborska mentioned this pull request Jun 13, 2021

Better documentation for contributing to the track (language versions, deps, configlet) #761

Merged

elibdev added 2 commits June 14, 2021 22:08

[exercism#722] New practice exercise yacht

bdb4f58

[exercism#722] Make yacht exercise example solution work with Elixi…

43049e3

…r 1.7

elibdev force-pushed the yacht-squash branch from e5749de to 43049e3 Compare June 15, 2021 02:16

angelikatyborska approved these changes Jun 15, 2021

View reviewed changes

Change string category to atom

6e1f624

Co-authored-by: Angelika Tyborska <[email protected]>

elibdev force-pushed the yacht-squash branch from 19e3d89 to 6e1f624 Compare June 15, 2021 11:40

angelikatyborska merged commit 4ab39ca into exercism:main Jun 15, 2021

angelikatyborska added reputation/contributed_code/major x:size/large Large amount of work labels Jun 15, 2021

angelikatyborska mentioned this pull request Jun 15, 2021

New practice exercise yacht #722

Closed

ErikSchierboom removed the reputation/contributed_code/major label Jun 29, 2021

This was referenced Jul 14, 2022

Document reputation label exercism/docs#347

Merged

Consider correct reputation amounts for different labels exercism/exercism#6440

Open

Consider correct reputation amounts for different labels exercism/exercism#6441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#722] New practice exercise `yacht` #760

[#722] New practice exercise `yacht` #760

elibdev commented Jun 13, 2021

github-actions bot commented Jun 13, 2021

neenjaw Jun 13, 2021

neenjaw Jun 13, 2021

elibdev Jun 13, 2021

neenjaw Jun 13, 2021

angelikatyborska Jun 13, 2021

elibdev Jun 13, 2021

neenjaw Jun 13, 2021

angelikatyborska Jun 13, 2021

elibdev Jun 13, 2021

neenjaw Jun 13, 2021

angelikatyborska Jun 13, 2021

elibdev Jun 13, 2021

angelikatyborska left a comment

angelikatyborska Jun 13, 2021

angelikatyborska Jun 13, 2021

angelikatyborska Jun 13, 2021

angelikatyborska Jun 13, 2021

elibdev Jun 13, 2021

elibdev commented Jun 13, 2021

angelikatyborska commented Jun 13, 2021

elibdev commented Jun 15, 2021

angelikatyborska left a comment

angelikatyborska Jun 15, 2021

angelikatyborska commented Jun 15, 2021

elibdev commented Jun 15, 2021

		@@ -0,0 +1,48 @@
		# Generate tests.toml file from canonical-data.json

	defp frequencies(strings) do
	# Enum.frequencies is only available from Elixir 1.10
	strings
	\|> Enum.reduce(%{}, fn word, count ->
	Map.update(count, word, 1, &(&1 + 1))
	end)
	end

[#722] New practice exercise yacht #760

[#722] New practice exercise yacht #760

Conversation

elibdev commented Jun 13, 2021

github-actions bot commented Jun 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelikatyborska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elibdev commented Jun 13, 2021

angelikatyborska commented Jun 13, 2021

elibdev commented Jun 15, 2021

angelikatyborska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelikatyborska commented Jun 15, 2021

elibdev commented Jun 15, 2021

[#722] New practice exercise `yacht` #760

[#722] New practice exercise `yacht` #760