[#725] New practice exercise `pov` #752

jiegillet · 2021-06-07T13:44:10Z

Dear maintainers,

Here is an extra practice exercise described in #725.
This is my first contribution, please help me get it right, I probably missed some things.

A couple of notes:

I didn't find a uuid for the exercise to add in config.json. How do I get it?
The test.tomls from a couple of other exercises I looked into looked self generated, but I don't know how the works, so I left the file blank for now.
The data structure I picked for the tree is pretty simple, it's just a tuple @type tree :: {any, [tree]}. Is it better to make it fancier, like a Map or a Struct?

Thank you!
Jeremie

github-actions · 2021-06-07T13:44:26Z

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 🤖.

config.json

neenjaw · 2021-06-07T13:52:25Z

exercises/practice/pov/.meta/example.ex

@@ -0,0 +1,133 @@
+defmodule Pov do


Since this exercise is basically Zipper + more functions, should see if we are being congruent with our approaches between the two exercises

Coincidentally, I just reached the Zipper exercise in my Elixir track. I'll go solve it real quick before looking into the repo ;)

OK, I've solved it now. The approach in Zipper is quite different from other exercises that I've seen so far since it provides a complete module for binary trees. It's not bad, should I model this one after Zipper?

I think we can start with your current implementation like it is now, and then later, if people complain that the exercise is too difficult, we could add hints in the form of a module with a data structure that we suggest using.

Agreed, challenging exercises are not as much of an issue for practice exercises

neenjaw

Thanks for the contribution! 💙 🎉

@angelikatyborska DM'd me saying she will do a proper review of your new exercise soon, I left a couple comments just to address a few points that stuck out to me.

Great work so far!

angelikatyborska

This is 🔥 amazing 🔥. Great work!

The test.tomls from a couple of other exercises I looked into looked self generated, but I don't know how the works, so I left the file blank for now.

In theory there should a tool for generating those files from problem specifications, but I don't know at the moment if it already exists 😅. You can copy the file from https://github.com/exercism/fsharp/blob/main/exercises/practice/pov/.meta/tests.toml -> this is exactly what we need. Those files list all tests from problem specifications by id and description and define if our track implements the specific test cases or not. You would mark a test as not being implemented (on purpose) with include = false.

I requested a tiny change to the test data (to use 0-indexing for siblings and cousins, and to write the whole word "sibling" instead of just "sub"). Otherwise it's perfect :)

exercises/practice/pov/test/pov_test.exs

angelikatyborska · 2021-06-07T19:08:39Z

config.json

+          "recursion",
+          "pattern-matching"
+        ],
+        "difficulty": 9


This is exciting 🤩 looks like this will be our most difficult exercise.

I have not done many of the difficulty 8, but it definitely is one step above Zipper, which is 8.
Forth is a 10 though!

I think difficult 9 is okay, we don't have a firm definition what each number means anymore, so it's just to give a general feel of the complexity

config.json

angelikatyborska · 2021-06-07T19:11:51Z

exercises/practice/pov/lib/pov.ex

+  @typedoc """
+  A tree, which is made of a node with several branches
+  """
+  @type tree :: {any, [tree]}


Great simple choice 👍

angelikatyborska · 2021-06-07T19:26:34Z

exercises/practice/pov/.meta/example.ex

+
+  defmodule Zipper do
+    defstruct [:focus, genealogy: []]
+    @type t :: %Zipper{focus: Pov.tree(), genealogy: [Crumb.t()]}


Dialyzer is complaining that it doesn't know the type Crumb.t. I think that can be fixed by first defining the Crumb module, and only then the Zipper module 🤔

angelikatyborska · 2021-06-07T19:32:29Z

exercises/practice/pov/test/pov_test.exs

+      sorted_a = Enum.sort(children_a)
+      sorted_b = Enum.sort(children_b)
+
+      Enum.zip_with(sorted_a, sorted_b, &equal_trees/2)


The CI is already reporting that tests don't pass in Elixir 1.8 😢. It's because this function was introduced in Elixir 1.12. To allow as many students as possible to solve the exercises, we have a CI check that runs the example solutions against different versions of Elixir, starting at 1.7. I think it won't be a problem to rewrite this with zip and map instead, right?

No problem indeed.

Co-authored-by: Angelika Tyborska <angelikatyborska@fastmail.com>

neenjaw

Nice work @jiegillet!! Really well done!

I'll let @angelikatyborska merge it when she is ready, but I have no hesitation reading through this.

P.S. for @angelikatyborska, will have to be squashed to merge because of some branch issue with the commits

jiegillet · 2021-06-08T02:53:29Z

Thank you very much for the welcome and the fast reviews!

jiegillet added 3 commits June 7, 2021 15:01

Add config files and test

2c7df41

WIP solution

ac66168

[exercism#725] New practice exercise pov

d39a481

neenjaw requested review from angelikatyborska and neenjaw June 7, 2021 13:45

neenjaw assigned jiegillet Jun 7, 2021

neenjaw added reputation/contributed_code/major v3 Related to Exercism v3 labels Jun 7, 2021

neenjaw reviewed Jun 7, 2021

View reviewed changes

config.json Outdated Show resolved Hide resolved

neenjaw reviewed Jun 7, 2021

View reviewed changes

config.json Outdated Show resolved Hide resolved

neenjaw reviewed Jun 7, 2021

View reviewed changes

Add uuid, match Zipper prerequisite, up difficulty

8989f5c

angelikatyborska approved these changes Jun 7, 2021

View reviewed changes

angelikatyborska reviewed Jun 7, 2021

View reviewed changes

jiegillet and others added 9 commits June 8, 2021 08:55

Merge branch 'jie-pov' of github.com:jiegillet/elixir into jie-pov

0917d03

Rename test parameters

0937c8c

Co-authored-by: Angelika Tyborska <angelikatyborska@fastmail.com>

lowercase uuid

9187afe

Co-authored-by: Angelika Tyborska <angelikatyborska@fastmail.com>

Test parameters renaming

2ea862e

Co-authored-by: Angelika Tyborska <angelikatyborska@fastmail.com>

Merge branch 'jie-pov' of github.com:jiegillet/elixir into jie-pov

11139f1

add tests.toml copied from fsharp

998d97b

Switch Crumb and Zipper modules

14251d8

Replace Enum.zip_with, rename equal_trees?

40af16a

Update collaborator list

7fe1b89

neenjaw approved these changes Jun 8, 2021

View reviewed changes

neenjaw linked an issue Jun 8, 2021 that may be closed by this pull request

New practice exercise pov #725

Closed

angelikatyborska merged commit 13eef73 into exercism:main Jun 8, 2021

ErikSchierboom added x:size/l and removed x:size/l labels Jun 8, 2021

jiegillet deleted the jie-pov branch June 12, 2021 14:07

ErikSchierboom added x:size/large Large amount of work and removed reputation/contributed_code/major labels Jun 25, 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

[#725] New practice exercise `pov` #752

[#725] New practice exercise `pov` #752

jiegillet commented Jun 7, 2021

github-actions bot commented Jun 7, 2021

neenjaw Jun 7, 2021

jiegillet Jun 7, 2021

jiegillet Jun 7, 2021

angelikatyborska Jun 7, 2021

neenjaw Jun 8, 2021

neenjaw left a comment

angelikatyborska left a comment

angelikatyborska Jun 7, 2021

jiegillet Jun 8, 2021

neenjaw Jun 8, 2021

angelikatyborska Jun 7, 2021

angelikatyborska Jun 7, 2021

angelikatyborska Jun 7, 2021

jiegillet Jun 8, 2021

neenjaw left a comment •

edited

Loading

jiegillet commented Jun 8, 2021

[#725] New practice exercise pov #752

[#725] New practice exercise pov #752

Conversation

jiegillet commented Jun 7, 2021

github-actions bot commented Jun 7, 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

neenjaw left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neenjaw left a comment • edited Loading

Choose a reason for hiding this comment

jiegillet commented Jun 8, 2021

[#725] New practice exercise `pov` #752

[#725] New practice exercise `pov` #752

neenjaw left a comment •

edited

Loading