Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New concept exercise top-secret (concept: ast) #930

Merged
merged 22 commits into from
Nov 6, 2021

Conversation

angelikatyborska
Copy link
Member

@jiegillet I loved your idea so much that I immediately started working on it :) this is just the example solution, I will work on the test suite next. Not necessary to review yet, but I just wanted to signal I'm working on it.

@jiegillet
Copy link
Contributor

Glad my idea inspired you so much :)
Maybe I can come up with a couple more test cases ^^

@angelikatyborska
Copy link
Member Author

@jiegillet the instructions, tests, stubs, and exemplar should all be "finished". It would be great if you could try to solve the exercise by following the instructions without looking at the exemplar first and let me know if they make sense, maybe even suggest hints based on your own experience 🙂

I will work on the introduction, hints, and everything else that needs to be finished soon.

@angelikatyborska angelikatyborska added the x:size/large Large amount of work label Oct 30, 2021
Copy link
Contributor

@jiegillet jiegillet left a comment

Choose a reason for hiding this comment

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

Awesome exercise. Here is my solution without looking at yours:

defmodule TopSecret do
  def to_ast(string) do
    Code.string_to_quoted!(string)
  end 

  @def_ops [:def, :defp]
  # this case I learned the hard way recently, but it might be too mean to include a case like this?
  def decode_secret_message_part({op, _, [{:when, _, [{name, _, args} | _]} | _]} = ast, acc)
      when op in @def_ops and is_list(args) do
    acc = add_part(name, length(args), acc)
    {ast, acc}
  end 

  def decode_secret_message_part({op, _, [{name, _, args} | _]} = ast, acc)
      when op in @def_ops and is_list(args) do
    acc = add_part(name, length(args), acc)
    {ast, acc}
  end 

  def decode_secret_message_part(ast, acc) do
    {ast, acc}
  end 

  def decode_secret_message(string) do
    {_, message} =
      string
      |> to_ast()
      |> Macro.prewalk([], &decode_secret_message_part/2)

    message
    |> Enum.reverse()
    |> Enum.join()
  end 

  defp add_part(name, arity, acc) do
    name
    |> to_string()
    |> String.slice(0, arity)
    |> then(&[&1 | acc])
  end 
end

@angelikatyborska
Copy link
Member Author

Hmm. Actually I like the idea of including a case for when as an extra task. It would increase the amount of actual AST work in the exercise, and together with the zero-arity-no-parentheses case it would drive the point of how non-trivial working with ASTs is when it needs to work with all kinds of inputs 🤔

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 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 (x:size/{tiny,small,medium,large,massive})? (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?
  • Concept exercise changed

    • 🌲 Do prerequisites and practices in config.json need to be updated?
    • 📖 Does the concept introduction provide all necessary information to solve this exercise?
  • Concept exercise tests changed

    • ⚪️ Are all tests un-skipped?
    • 🔢 Are all tests annotated with @tag task_id?
    • 🐈 Can all tests be understood by reading the test's block only (e.g. no module attributes, no setup functions)?
  • Concept changed

    • 👤 Does the author of the PR need to be added as an author or contributor in <concept>/.meta/config.json (see docs)?
    • 📖 Does the concept introduction provide all necessary information to solve this exercise?

Automated comment created by PR Commenter 🤖.

@angelikatyborska angelikatyborska marked this pull request as ready for review November 4, 2021 18:38
@@ -0,0 +1,40 @@
# About
Copy link
Member Author

Choose a reason for hiding this comment

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

I gave up on writing a custom about.md and just copy-pasted introduction.md because I have doubts about how many students actually read those and I'm low on time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we will need to circle back to it once we have quote/unquote and macros concepts anyway.

Copy link
Contributor

@jiegillet jiegillet left a comment

Choose a reason for hiding this comment

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

Looks great!
I left a couple of comments for small things.


The ability to represent code as an AST is at the heart of metaprogramming in Elixir. _Macros_, which is a way to write Elixir code that produces Elixir code, work by returning ASTs as output.

Another use case for ASTs is static code analysis, like Exercism's own tool, the Analyzer, which you might already known as the little bot that leaves comments on your solutions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Another use case for ASTs is static code analysis, like Exercism's own tool, the Analyzer, which you might already known as the little bot that leaves comments on your solutions.
Another use case for ASTs is static code analysis, like Exercism's own tool, the Analyzer, which you might already know as the little bot that leaves comments on your solutions.

@@ -0,0 +1,40 @@
# About
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we will need to circle back to it once we have quote/unquote and macros concepts anyway.

@@ -596,6 +596,23 @@
"dates-and-time"
],
"status": "beta"
},
{
"slug": "top-secret",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add the prerequisite to dot-dsl too.

- The AST node that contains the function's name also contains the function's argument list as the third element.
- The exact pattern to match an AST node that defines a function and get its name and arguments is: `{op, _, [{name, _, arguments} | _]} when op in [:def, :defp]`.
- The arity of a function is the length of its argument list.
- There is a [built-in function in the `String` module][string-slice] that can get the first `n` characters from a string.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add one more hint:

Suggested change
- There is a [built-in function in the `String` module][string-slice] that can get the first `n` characters from a string.
- There is a [built-in function in the `String` module][string-slice] that can get the first `n` characters from a string.
- A function without arguments written without parentheses will not have a list as argument but an atom.


## 5. Decode the full secret message

- There is a [built-in function][macro-prewalk] that can visit each node in an AST with an accumulator.
Copy link
Contributor

Choose a reason for hiding this comment

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

One more:

Suggested change
- There is a [built-in function][macro-prewalk] that can visit each node in an AST with an accumulator.
- There is a [built-in function][macro-prewalk] that can visit each node in an AST with an accumulator.
- Use the function `to_ast/1` that you implemented in the first task to create the AST.

@angelikatyborska
Copy link
Member Author

I'll merge this in an hour or so 🤞 it feels much more stressful to add new exercises when v3 is already live.

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.

None yet

2 participants