Skip to content

Conversation

@jiegillet
Copy link
Contributor

I had fun with the previous PR, so here I go again with #723 .

It was pretty tough to solve, definitely the one that took me the longest out of all the exercises on Exercism that I have done so far. I would say that the algorithm that I came up with is complex, but the Elixir tools required to solve are fairly simple.

Please have a look :)

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2021

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

Copy link
Member

@angelikatyborska angelikatyborska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOW 😍 I did not expect another PR so soon. Amazing work again! And another very difficult exercise!

The CI is going to complain a bit about formatting. I think you need to do two or three changes:

  1. Run ./bin/check_formatting.sh to find trailing whitespace and remove it
  2. Run mix format on the whole exercise
  3. Run mix format on .meta/example.ex, and for that I think you need to temporarily move it to lib and back.

If you don't manage to please the CI's formatting checks, I can help out and add a commit :) I can be a bit annoying.

@@ -0,0 +1,192 @@
defmodule ZebraPuzzle do
@nationalities ~w(englishman norvegian ukrainian japanese spaniard)a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small typo (also in the test):

Suggested change
@nationalities ~w(englishman norvegian ukrainian japanese spaniard)a
@nationalities ~w(englishman norwegian ukrainian japanese spaniard)a

config.json Outdated
"cond",
"if",
"enum",
"keyword-lists",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see keyword lists in your solution, but I did see maps. Do you think we should swap this prerequisite for maps? Or should we have both? I'm also seeing recursion in your solution. Maybe that should also be a prerequisite?

A prerequisite doesn't mean you have to use that concept to solve the exercise, but it will stop people from solving it before they do know the concept (so that they can properly choose, for example enum vs recursion, or both).

@neenjaw thoughts?

Copy link
Contributor Author

@jiegillet jiegillet Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I did sort of use keyword lists at some point because when you enumerate on a map it turns into a keyword list, but I changed the implementation to use Map.new() instead, so I should remove this. I will swap the requirement with maps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think swapping the pre-req with maps is a good step for this. While you are right, enumerated maps and key-word lists elements appear similarly I would hesitate to say that maps are transformed into keyword lists, the tuple-pair is the focus of that I think.

Personally I am okay with hard exercises appearing early in the exercise progression as long as the student has the tools to solve it. So i would just make sure that there isn't a hidden pattern in the code required to solve that would be taught more explicitly with another more "advanced" concept

end

def filter_by_unique_relations(list) do
# Some values happen to exist only in one particulat house number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably typo?

Suggested change
# Some values happen to exist only in one particulat house number
# Some values happen to exist only in one particular house number

end)
|> Enum.concat()

# Add those values as contraints and filter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably typo?

Suggested change
# Add those values as contraints and filter
# Add those values as constraints and filter

@jiegillet
Copy link
Contributor Author

jiegillet commented Jun 10, 2021

OK, I think I fixed the CI issues. What gave me most trouble was realizing that the --warnings-as-errors flag was on, and I still had 2 warnings (I used solve_puzzle instead of solve_puzzle(), usually mix format takes care of brackets for me, but I guess not this time). Good to know!

Also in my debugging process, I had to fiddle with the sed command in ./bin/test_exercises.sh because I'm on a Mac. I'll share my modifications, maybe it's worth adding to the file as comments?

-      module_name=$(cat "${test_file}" | sed -rn 's/^defmodule (.*)Test do$/\1 /p')
+      module_name=$(cat "${test_file}" | sed -En 's/^defmodule (.*)Test do$/\1 /p')
       doctest_code="doctest ${module_name}"
 
       # Warning: GNU sed necessary, BSD (macOS) sed has incompatible options
-      sed -i 's/use ExUnit.Case\(.*\)/use ExUnit.Case\1\n'" ${doctest_code}"'\n/g' "${test_file}"
+      sed -i '' -E $'s/use ExUnit.Case(.*)/use ExUnit.Case\\1'$'\\\n'"  ${doctest_code}"$'\\\n/g' ${test_file}

@neenjaw
Copy link
Contributor

neenjaw commented Jun 10, 2021

Cool use of sed to trim out the extra whitespace! I would prefer it not added to the test file source though, let me think if we have a "useful snippets" document which would be a good place for that

@jiegillet
Copy link
Contributor Author

Oh this doesn't trim out the whitespace, it lets bin/pr.sh run on macOS.

@neenjaw
Copy link
Contributor

neenjaw commented Jun 10, 2021

Ah! Sorry, I misread the context of the sed. 😳 Yes, a separate PR to add those as comments would be fine

Comment on lines +2397 to +2398
"enum",
"cond"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure that adding these doesn't put us over the 8 exercise limit for the concept exercise @angelikatyborska

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely forgot about this 😱 was the limit 8 or 10? If the limit is 8, then we need to rethink 7 concepts. I think we can do that on a separate branch. I wouldn't want to block @jiegillet PR because of our previous carelessness 😅

~*~*~*~ enum ~*~*~*~

Practiced by:
10
[
  'rna-transcription',
  'resistor-color',
  'raindrops',
  'sum-of-multiples',
  'anagram',
  'hamming',
  'transpose',
  'connect',
  'minesweeper',
  'zebra-puzzle'
]

~*~*~*~ integers ~*~*~*~

Practiced by:
9
[
  'collatz-conjecture',
  'nth-prime',
  'leap',
  'all-your-base',
  'change',
  'luhn',
  'armstrong-numbers',
  'largest-series-product',
  'clock'
]

~*~*~*~ maps ~*~*~*~

Practiced by:
9
[
  'nucleotide-count',
  'scrabble-score',
  'grade-school',
  'matrix',
  'tournament',
  'binary-search-tree',
  'kindergarten-garden',
  'custom-set',
  'saddle-points'
]

~*~*~*~ pattern-matching ~*~*~*~

Practiced by:
11
[
  'bowling',
  'protein-translation',
  'triangle',
  'tournament',
  'meetup',
  'perfect-numbers',
  'queen-attack',
  'poker',
  'alphametics',
  'wordy',
  'pov'
]

~*~*~*~ ranges ~*~*~*~

Practiced by:
10
[
  'rotational-cipher',
  'pangram',
  'sum-of-multiples',
  'nth-prime',
  'isbn-verifier',
  'grains',
  'rail-fence-cipher',
  'diffie-hellman',
  'palindrome-products',
  'difference-of-squares'
]

~*~*~*~ recursion ~*~*~*~

Practiced by:
11
[
  'roman-numerals',
  'strain',
  'accumulate',
  'matching-brackets',
  'binary-search',
  'binary-search-tree',
  'prime-factors',
  'pascals-triangle',
  'spiral-matrix',
  'sieve',
  'pov'
]

~*~*~*~ regular-expressions ~*~*~*~

Practiced by:
9
[
  'word-count',
  'markdown',
  'forth',
  'acronym',
  'run-length-encoding',
  'isogram',
  'phone-number',
  'isbn-verifier',
  'grep'
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@neenjaw neenjaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking at your solution and I wonder if there is a way to solve this using list comprehensions: generators to create the possibilities and filters to select the possible answers, but that said, the purpose of an example solution is to prove that it is solvable in elixir, not to have the best solution and yours works! 🎉

I'm thrilled to have yet another exercise to solve on this track!

Thanks @jiegillet!! You're on fire! 🔥

@jiegillet
Copy link
Contributor Author

jiegillet commented Jun 10, 2021

That's true, list comprehensions would make the possibilities creation cleaner, and possibly filters. I haven't used list comprehensions in Elixir very much yet.

I'll look forward to seeing your solution on Exercism 🤣

I'll probably do more remaining practice exercises.

@angelikatyborska
Copy link
Member

@jiegillet About sed, there is a warning in the script (that I totally forgot about until now) that on a mac, you need to install GNU sed. Did you do that? I did, and I never had problems with the scripts. All I know is that GNU sed and BSD sed are somehow incompatible, but I'm not sure exactly how. Could that explain the problems you had with the script?

It would be great if we had a single script that could detect which sed is present and adjust the sed call accordingly 🤔

@jiegillet
Copy link
Contributor Author

The incompatibilities are things like different flags (GNU -r == BSD -E) or even different behaviors (-i for writing inline works without options on GNU but needs an empty string -i '' on BSD, also the \n characters don't work directly on BSD). Kind of a pain. And, in hindsight, I should have tried installing GNU sed.

I'm not sure how to detect sed version, I can look into it and open a PR if it's worth it.

@angelikatyborska
Copy link
Member

angelikatyborska commented Jun 10, 2021

I'm not sure how to detect sed version, I can look into it and open a PR if it's worth it.

If this something you're interested in doing, that would be great. If not, @neenjaw and I will figure something out.

@neenjaw
Copy link
Contributor

neenjaw commented Jun 10, 2021

🤷‍♂️ I'm not sure it's really worth doing to automate it when it is mostly used by CI which runs on ubuntu. Just having the bsd-compatible form in a comment is all I'm looking for.

@angelikatyborska
Copy link
Member

OK, there's no reason to delay merging this! Let's merge and deal with the "practices" limit per concept in a separate PR.

@angelikatyborska
Copy link
Member

Thank you again @jiegillet ❤️ 🧡 💛 💚 💙 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

x:size/large Large amount of work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants