Skip to content

Conversation

@jiegillet
Copy link
Contributor

One more. I really enjoyed coming up with a clean algorithm for this one!

The metadata file was missing a source and URL.

@github-actions
Copy link
Contributor

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

Looks good by me!

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.

❤️ 🧡 💛 💚 💙 💜

@doc """
Build a tree from the elements given in a pre-order and in-order style
"""
@spec build_tree(preorder :: [any], inorder :: [any]) :: tree | {:error, String.t()}
Copy link
Member

Choose a reason for hiding this comment

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

I forgot to pay attention to this in the previous PRs: the more "Elixir way" would be to return:

{:ok, tree} | {:error, String.t()}

Usually functions that return an error tuple when something goes wrong, return an ok-tuple when everything is fine (a few examples: File.read, DateTime.new, Ecto.Repo.insert). It would be nice if our exercises followed that convention too.

It would be great if you did that change in POV too. Better in a separate PR, that will give you extra reputation points 😬 (reputation is a new feature in v3, it's just some points that you can collect but not really use for anything).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Actually at some point I had an implementation that used {:ok, tree} and all the checks along the way used with instead of using MapSet but I found it a bit bulky. It was the better design in the end.

I'll revisit POV too, what wouldn't I do for useless internet points :)

@angelikatyborska
Copy link
Member

And another one getting merged 🚀 🎉

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