New practice exercise `sgf-parsing` #795

jiegillet · 2021-06-28T14:18:26Z

New one!

This one might need a bit of discussion because I went a little nuts here. I love using parser combinators in Elm and Haskell, so whenever I see a slightly non-trivial parsing problem, I want to use that. I believe there are such libraries in Elixir (NimbleParsec judging from this article that inspired me) but I felt like building my own, so I did.
It was super fun, but I feel that most people will not use that technique, they would probably use regex or something. What is the go-to parsing method in Elixir?

In any case, this might be acceptable because it's only an example implementation, but that makes it hard to pick good prerequisites and a difficulty level. What do you think?

github-actions · 2021-06-28T14:18:41Z

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

angelikatyborska · 2021-06-28T14:55:05Z

I love using parser combinators in Elm and Haskell, so whenever I see a slightly non-trivial parsing problem, I want to use that.
(...)
What is the go-to parsing method in Elixir?

Honestly, this is a bit above my knowledge level 🙈. I'm just a simple web developer who doesn't know fancy algorithms... it's embarrassing but I have successfully avoided solving the Markdown exercise so far 🙈 @neenjaw how is your advanced string parsing in Elixir?

I believe there are such libraries in Elixir but I felt like building my own, so I did.

That's a good call because you couldn't use an external dependency in a solution anyway 😉

In any case, this might be acceptable because it's only an example implementation

It works, it fulfills the requirements, I don't understand it at all so I can't say more 😅 but it's fine with me

but that makes it hard to pick good prerequisites and a difficulty level. What do you think?

That probably means we should require all of the usual things :) difficulty level 8 also sounds reasonable to me.

I'm only conflicted about the "errors" concept. I think it should be defined as "raising errors" and as such shouldn't be part of this exercise or circular-buffer. Sorry that I wasn't decisive about this earlier. But now that I look at the config, only exercises that explicitly raise something or have a test assertions about raising "practice" errors. The ok/error tuple pattern is introduces in the "tuples" concept.

Nitpick: there's some wild .config.json.swo file 👀

neenjaw · 2021-06-28T15:17:21Z

exercises/practice/sgf-parsing/.docs/instructions.md

+    B\[aa\].  (Black plays on the point encoded as "aa", which is the
+    1-1 point (which is a stupid place to play)).


Is this really in the problem specs instructions?

Suggested change

B\[aa\]. (Black plays on the point encoded as "aa", which is the

1-1 point (which is a stupid place to play)).

B\[aa\]. (Black plays on the point encoded as "aa", which is the

1-1 point).

If it is, I think this needs an upstream PR

exercism/problem-specifications#1814

I personally find it pretty amusing :)

Ya, but we've made a conscious effort to remove this type of comment/test case from problems.

Yes, that's probably the right decision. What do I do for now? Remove it here and you do a PR? Or do we wait for your PR?
Is there a reason why the specifications are duplicated in the tracks repositories? I would think that a central one might work best, with appendices added by the tracks as necessary, which is kind of what's happening now anyway.

Problem specs will take time to approve, and afaik it doesn't sync yet. I think there is a copy for the same reason as right now it doesn't pull the instructions centrally. I think this is future work to be done, just hasn't been prioritized ahead of other product work for v3.

neenjaw

Cool stuff, I've seen and used NimbleCSV and NimbleParsec before never implemented it myself. Left a couple comments just what I thought of the example while reading it. Given it is a proof of solvability, all non-blocking for me.

Great work

neenjaw · 2021-06-28T15:44:13Z

exercises/practice/sgf-parsing/.meta/example.ex

+    lift2(
+      &{&1, &2},
+      some(satisfy(&(&1 not in '[();')))
+      |> map(&Enum.join(&1, ""))
+      |> validate(&(&1 == String.upcase(&1)), "property must be in uppercase"),
+      some(
+        char(?[)
+        |> error("properties without delimiter")
+        |> drop_and(many(escaped(&(&1 != ?]))))
+        |> drop(char(?]))
+        |> map(&Enum.join(&1, ""))
+      )
+    )


I think it would be easier to read/understand this block if it were split apart,

Suggested change

lift2(

&{&1, &2},

some(satisfy(&(&1 not in '[();')))

|> map(&Enum.join(&1, ""))

|> validate(&(&1 == String.upcase(&1)), "property must be in uppercase"),

some(

char(?[)

|> error("properties without delimiter")

|> drop_and(many(escaped(&(&1 != ?]))))

|> drop(char(?]))

|> map(&Enum.join(&1, ""))

)

)

node_property =

some(satisfy(&(&1 not in '[();')))

|> map(&Enum.join(&1, ""))

|> validate(&(&1 == String.upcase(&1)), "property must be in uppercase")

node_value =

some(

char(?[)

|> error("properties without delimiter")

|> drop_and(many(escaped(&(&1 != ?]))))

|> drop(char(?]))

|> map(&Enum.join(&1, ""))

)

lift2(&{&1, &2}, node_property, node_value)

neenjaw · 2021-06-28T15:48:35Z

exercises/practice/sgf-parsing/.meta/example.ex

+      with {:ok, result, rest} <- parser.(string) do
+        if p.(result) do
+          {:ok, result, rest}
+        else
+          {:error, err, rest}
+        end
+      end


When there is a single expression in the with, is there a benefit to using with over just a match?

Suggested change

with {:ok, result, rest} <- parser.(string) do

if p.(result) do

{:ok, result, rest}

else

{:error, err, rest}

end

end

{:ok, result, rest} = parser.(string)

if p.(result) do

{:ok, result, rest}

else

{:error, err, rest}

end

The with here is important, because I need the error to be passed further if parser.(string) fails. If I use {:ok, result, rest} = parser.(string) and the parser fails, I will get an exception.

neenjaw · 2021-06-28T15:56:10Z

The *.swo file is a vim swap file, accidental inclusion I would think

neenjaw · 2021-06-28T16:02:26Z

What is the go-to parsing method in Elixir?

For beginner/intermediate, which is what we are likely to see on exercism, I would guess that most people will end up using a combination of string lib functions and regex.

jiegillet · 2021-06-29T00:46:09Z

Nitpick: there's some wild .config.json.swo file 👀

The *.swo file is a vim swap file, accidental inclusion I would think

Yes, I'm ashamed 😳

For beginner/intermediate, which is what we are likely to see on exercism, I would guess that most people will end up using a combination of string lib functions and regex.

OK, so leaving regular-expressions in the prerequisites in the right call.

it's embarrassing but I have successfully avoided solving the Markdown exercise so far 🙈

Give it a try, it's not that hard, because you just need to refactor existing code :)

I'm only conflicted about the "errors" concept. I think it should be defined as "raising errors" and as such shouldn't be part of this exercise or circular-buffer. Sorry that I wasn't decisive about this earlier. But now that I look at the config, only exercises that explicitly raise something or have a test assertions about raising "practice" errors. The ok/error tuple pattern is introduces in the "tuples" concept.

OK, I will remove errors from the list. But for this particular exercise, handling errors was actually tricky, and I would call that proper handling practice, so I wish we had a concept for that.

angelikatyborska · 2021-06-29T15:22:18Z

I would call that proper handling practice, so I wish we had a concept for that.

Maybe it's a topic we could explore more as part of the with concept? (Not yet implemented)

Speaking of which, if (when? 😇) we ever add that with concept, should that exercise require it and practice it? If yes, I will edit the description of #560 to mention that

jiegillet · 2021-06-30T00:52:40Z

I think we should touch on it with the with concept. Now I'm having too much fun doing the exercises, but when they (soon) run out, I'm planning on taking a look at concepts. I would also like to have a reduce concepts, it's so powerful and so underused (looking at other students' code).

However, I'm not sure this exercise is the best to link with the with concept. It's crucial to my implementation, but I would like to see other solutions first.

angelikatyborska · 2021-06-30T07:07:21Z

It would be great to have more help with concepts and concept exercises 🙂

reduce is part of the enum concept:

introduction (available before solving a concept exercise):

elixir/concepts/enum/introduction.md

Lines 14 to 20 in 9774308

    
           The most common `Enum` functions are `map` and `reduce`. 
        
           `Enum.map/2` allows you to replace every element in an enumerable with another element. The second argument to `Enum.map/2` is a function that accepts the original element and returns its replacement. 
        
           `Enum.reduce/2` allows you to _reduce_ the whole enumerable to a single value. To achieve this, a special variable called the _accumulator_ is used. The accumulator carries the intermediate state of the reduction between iterations. 
        
           The second argument to `Enum.reduce/2` is the initial value of the accumulator. The third argument is a function that accepts an element and an accumulator, and returns the new value for the accumulator.

about (avaialbe after solving a concept exercise):

elixir/concepts/enum/about.md

Lines 33 to 61 in 9774308

    
           ## Reduce 
        
           `Enum.reduce/2` allows you to _reduce_ the whole enumerable to a single value. To achieve this, a special variable called the _accumulator_ is used. The accumulator carries the intermediate state of the reduction between iterations. This makes it one of the most powerful functions for enumerables. Many other specialized functions could be replaced by the more general `reduce`. For example... 
        
           Finding the maximum value: 
        
           ```elixir 
        
           Enum.max([4, 20, 31, 9, 2]) 
        
           # => 31 
        
           Enum.reduce([4, 20, 31, 9, 2], nil, fn x, acc -> 
        
             cond do 
        
               acc == nil -> x 
        
               x > acc -> x 
        
               x <= acc -> acc 
        
             end 
        
           end) 
        
           # => 31 
        
           ``` 
        
           And even mapping (but it requires reversing the result afterwards): 
        
           ```elixir 
        
           Enum.map([1, 2, 3, 4, 5], fn x -> x + 10 end) 
        
           # => [11, 12, 13, 14, 15] 
        
           Enum.reduce([1, 2, 3, 4, 5], [], fn x, acc -> [x + 10 | acc] end) 
        
           # => [15, 14, 13, 12, 11] 
        
           ```

angelikatyborska · 2021-06-30T07:13:46Z

It looks to me like @neenjaw's feedback has been all addressed, so I'm merging 🎉

New practice exercise

69a459a

neenjaw reviewed Jun 28, 2021

View reviewed changes

neenjaw approved these changes Jun 28, 2021

View reviewed changes

jiegillet added 7 commits June 29, 2021 10:34

Remove errors

f9694b9

Update instructions

38d1bda

Refactor examples.ex

8cff89d

Remove .swo

c47fabb

Contributors

65749c3

Merge branch 'main' into jie-sgf-parsing

d70d76b

Remove practices string (check passed before food-chain was merged)

6884881

neenjaw approved these changes Jun 29, 2021

View reviewed changes

angelikatyborska added the x:size/large Large amount of work label Jun 30, 2021

angelikatyborska approved these changes Jun 30, 2021

View reviewed changes

angelikatyborska merged commit b9432de into exercism:main Jun 30, 2021

angelikatyborska mentioned this pull request Jun 30, 2021

New practice exercise sgf-parsing #790

Closed

jiegillet deleted the jie-sgf-parsing branch July 1, 2021 00:06

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

New practice exercise `sgf-parsing` #795

New practice exercise `sgf-parsing` #795

jiegillet commented Jun 28, 2021

github-actions bot commented Jun 28, 2021

angelikatyborska commented Jun 28, 2021

neenjaw Jun 28, 2021 •

edited

Loading

angelikatyborska Jun 28, 2021

jiegillet Jun 29, 2021

neenjaw Jun 29, 2021

jiegillet Jun 29, 2021

neenjaw Jun 29, 2021

neenjaw left a comment

neenjaw Jun 28, 2021

neenjaw Jun 28, 2021 •

edited

Loading

jiegillet Jun 29, 2021

neenjaw Jun 29, 2021

neenjaw commented Jun 28, 2021

neenjaw commented Jun 28, 2021

jiegillet commented Jun 29, 2021

angelikatyborska commented Jun 29, 2021

jiegillet commented Jun 30, 2021

angelikatyborska commented Jun 30, 2021

angelikatyborska commented Jun 30, 2021

		B\[aa\]. (Black plays on the point encoded as "aa", which is the
		1-1 point (which is a stupid place to play)).

New practice exercise sgf-parsing #795

New practice exercise sgf-parsing #795

Conversation

jiegillet commented Jun 28, 2021

github-actions bot commented Jun 28, 2021

angelikatyborska commented Jun 28, 2021

neenjaw Jun 28, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neenjaw Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neenjaw commented Jun 28, 2021

neenjaw commented Jun 28, 2021

jiegillet commented Jun 29, 2021

angelikatyborska commented Jun 29, 2021

jiegillet commented Jun 30, 2021

angelikatyborska commented Jun 30, 2021

angelikatyborska commented Jun 30, 2021

New practice exercise `sgf-parsing` #795

New practice exercise `sgf-parsing` #795

neenjaw Jun 28, 2021 •

edited

Loading

neenjaw Jun 28, 2021 •

edited

Loading