New practice exercise `react` #820

jiegillet · 2021-07-06T14:30:52Z

This is one a bit harder than the last two. Even combined :)

I would welcome your opinion especially concerning the API, choice of functions, return types and such. For example, I'm sending back the callbacks in a map, but I'm not sure that is idiomatic.

I chose to use cell names as strings instead of atoms, because I know that atoms are not garbage collected and this felt like the kind of system where you would programmatically create new cells, even though it doesn't happen here. But atoms are cleaner, so I'm on the fence.

I left some of the test comments in, because they gave me some insight into the problem, but I can remove them if needed.

Difficulty around 8 or 9 sounds right, but I think I'm mixing the difficulty of designing the tests.

New practice exercise `react` New practice exercise `react` New practice exercise `react` New practice exercise `react` Change return type

github-actions · 2021-07-06T14:31:06Z

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

That's a really fascinating exercise 🤔 I'm really curious how people will approach it, and I'm very happy we have more genserver!

angelikatyborska · 2021-07-07T07:13:12Z

config.json

+          "recursion"
+        ],
+        "practices": [
+          "processes"


I made a note in #559 that this will be a genserver exercise once we have that concept 🙂

angelikatyborska · 2021-07-07T07:23:52Z

exercises/practice/react/.meta/example.ex

+    defstruct [:name, :inputs, :compute, :value, callbacks: []]
+
+    @type t :: %OutputCell{
+            name: String.t(),


~~Do you think cell names should be atoms instead?~~

Ooops, didn't see the PR description before posting.

I chose to use cell names as strings instead of atoms, because I know that atoms are not garbage collected and this felt like the kind of system where you would programmatically create new cells, even though it doesn't happen here. But atoms are cleaner, so I'm on the fence.

I agree with the concern, let's keep strings. Maybe you could describe this in .meta/design.md? for future maintainer generations :)

That's a good suggestion, I will do that. Those notes are only meant for maintainers right?

angelikatyborska · 2021-07-07T07:26:52Z

exercises/practice/react/test/react_test.exs

+  test "callbacks should not be called if dependencies change but output value doesn't change" do
+    # Some incorrect implementations simply mark a compute cell as dirty when a dependency changes,
+    # then call callbacks on all dirty cells.
+    # This is incorrect since the specification indicates only to call callbacks on change.


I think those comments are very helpful for understanding the exercise

angelikatyborska · 2021-07-07T07:34:53Z

exercises/practice/react/lib/react.ex

+            name: String.t(),
+            inputs: [String.t()],
+            compute: (... -> any),
+            value: any,


If we can allow cells to have any type, should we add a custom test case that tests string cells for example? or boolean cells? (but no error handling of type mismatch)

Problem specifications say:

If your language supports generics, you may consider allowing reactors to act on other types.
Tests for that are not included here.

Ah, sure, why not!
Any particular procedure for adding tests?

Since those are our own, we don't need to add anything to the toml file. Maybe mention that we did that in design.md but otherwise nothing special.

angelikatyborska · 2021-07-07T07:35:30Z

exercises/practice/react/lib/react.ex

+  end
+
+  @doc """
+  Set the value of an input cell


Maybe this needs a hint about what the function returns?

Ah, the callbacks. Yes, good idea.
Also I have a question about this. It is specified that you can only have specific callback once, so I picked a map as the data structure since the uniqueness of the keys would always enforce that. But that seems like a decision that has to be made by the student. Of course, it's also important for the tests so... I'm not sure.

After some thought, I find it a bit weird that there are no actual functions involved in the callbacks 🤔.
I'm thinking the API should rather be like this:

React.add_callback(cells, "always_two", fn value -> send(self(), {:callback1, value})) :ok = React.set_value(cells, "input", 2) refute_received {:callback, _}, "Expected callback not to be called" # maybe the failure message can be improved

Ah nice!
I actually don't have a good intuition of what callbacks are, so this is very helpful. This feels closer to what it should be!

I had a go at something that I found sensible, please check :)

Looks much better 👍

neenjaw

I think this is supposed to be a difficult problem, and I'm okay with it being difficult. I think though, we are perhaps prescribing a very specific approach by defining the structs and using them in the test file.

For me, I'd like to see the interface be a little more agnostic to the solution's data model. A more generic input and allow the student to flex some creativity in their own solution. (To be clear, there is certainly nothing wrong with the approach @jiegillet has taken with their solution to use structs for this, just not sure it should be mandated for every student)

E.g.

# input format
{:input, "input", 1}
# output format
{:output, "output", ["input"], fn a -> a + 1 end}

# Then the interface for the test file becomes:
test "compute cells calculate initial value" do
  {:ok, cells} =
    React.new([
      {:input, "input", 1},
      {:output, "output", ["input"], fn a -> a + 1 end}
    ])

  assert React.get_value(cells, "output") == 2
end

The format could be implemented as a provided type.

Interested in your thoughts

jiegillet · 2021-07-08T06:48:56Z

That makes sense. I use Haskell and Elm a lot, so I'm very type-driven and that's how I structure code, but I should let students make their own design choices. The change you propose is pretty straightforward too, so no problem.

angelikatyborska · 2021-07-08T12:11:03Z

exercises/practice/react/lib/react.ex

+  def add_callback(cells, cell, callback, send) do
+  end


I think we could improve argument names in the whole stub file E.g. I'm not entirely sure about the name "send". It's specific to message sending, but the callback can do anything. In practice it only makes sense if it does side-effect (because there's nobody that cares about the return value) but the side effects could be not only message sending, but also doing network requests or something 🤷

I would propose:
cell -> cell_name (because this is the string, not the map %{type: :input, value: 1} and definitely not a single instance of the first argument, cells)
callback -> callback_name (because "callback" usually means it's a function)
send -> callback or function or callback_fun?

angelikatyborska · 2021-07-08T12:15:37Z

exercises/practice/react/lib/react.ex

+          cells :: pid,
+          cell :: String.t(),
+          callback :: String.t(),
+          send :: (String.t(), any -> :ok)


Does it really matter what the callback returns?

Well the callback must send a message, which is of type :ok, otherwise the tests can't be passed.

Must send a message? The callback we use in tests sends a message so that we have an easy way of verifying that it was called, but in general the callback can do anything. It's even in your implementation:

Enum.each(cell.callbacks, fn {name, send} -> send.(name, value) end)

Enum.each -> return value gets discarded, only side effects matter

Right, I get it.

I think what I was doing here is matching the type of the actually callbacks , which are all predefined in the tests as send_callback and are indeed all send, but I get what you are saying, in a real API, you wouldn't restrict the callback type. I will change it :)

angelikatyborska · 2021-07-08T12:18:48Z

exercises/practice/react/lib/react.ex

+  end
+
+  @doc """
+  Set the value of an input cell


Looks much better 👍

angelikatyborska · 2021-07-08T12:21:29Z

exercises/practice/react/lib/react.ex

+  @doc """
+  Start a reactive system
+  """
+  @spec new(cells :: [%{atom => any}]) :: {:ok, pid}


The map %{atom => any} will be a very specific map, right? One of:

%{type: :input, name: String.t(), value: any()} %{type: :output, name: String.t(), inputs: [String.t()], compute: (...) -> any()}

Should we write that in the typespec instead of the generic %{atom => any}?

neenjaw

I think this is a good iteration, but I still think a tuple is preferable to a map/struct for the arguments

neenjaw · 2021-07-08T13:25:35Z

exercises/practice/react/test/react_test.exs

+        %{type: :input, name: "input", value: 1},
+        %{type: :output, name: "output", inputs: ["input"], compute: fn a -> a + 1 end}


I understand why you are using a map in your data model, to enforce a single entry per key, but I think when sending messages it is much more common to use a tuple for this.

Suggested change

%{type: :input, name: "input", value: 1},

%{type: :output, name: "output", inputs: ["input"], compute: fn a -> a + 1 end}

{:input, "input", 1},

{:output, "output", ["input"], fn a -> a + 1 end}

Actually, this is not why I don't use a tuple, here I use a map because I want to have named fields, otherwise it's unnecessarily complicated for the student to figure out which value is what... This is just following the data from canonical-data.json.

If there was only two fields, it would be ok, but if I have sometimes 2, sometimes 3, I would always go for clarity.
If you insist I will change to tuples, though.

Well, maybe with the @spec specifying which is what it would be OK.

I would rather we keep the maps precisely to have named fields. Tuples will be more confusing IMO, because there will be two different ones of different lengths...

but I think when sending messages

This is not sending messages 🤔

I mean "send a message" in a more philospohical sense (archived article since my link is dead (beware, they use the dreaded OOP word, but I think it as a philosophy still stands)). Calling a function is "sending a message". Eg. when invoking the function new I am sending a message to create a system with these constraints.

Tuples for this sort of thing is still super common (especially in erlang), and we see tuples of varying lengths for various meanings all the time: {:ok, value1, value2} vs {:error, "message"}

But that's why I suggested having a defined type for the tuple so that you can give meaning to the tuple

How about we compromise:

%{type: :input, name: "input", value: 1}, {:output, "output", ["input"], fn a -> a + 1 end}

(🤣 I'm very sorry 😆)

I will reflect on it for a bit. Thank you both for the detailed review.

Hmm, I see. Ok, I won't insist on maps, if @neenjaw is sure tuples will be better in this context, they will work too if we write a nice spec.

angelikatyborska · 2021-07-09T10:43:50Z

I think it's good to merge from my side 🎉 let's see what @neenjaw says

neenjaw

Looks great! 🎉

neenjaw · 2021-07-10T01:42:33Z

exercises/practice/react/lib/react.ex

+  @doc """
+  Start a reactive system
+  """
+  @spec new(cells :: [{:input, String.t(), any} | {:output, String.t(), [String.t()], fun()}]) ::
+          {:ok, pid}
+  def new(cells) do
+  end


You could create a type here to break down this complex type

Suggested change

@doc """

Start a reactive system

"""

@spec new(cells :: [{:input, String.t(), any} | {:output, String.t(), [String.t()], fun()}]) ::

{:ok, pid}

def new(cells) do

end

@type input :: {:input, name :: String.t(), value :: any}

@type output ::

{:output, name :: String.t(), dependencies :: list(String.t()), callback :: function()}

@type cell_definition :: input() | output()

@opaque cells :: pid

@doc """

Start a reactive system

"""

@spec new(cell_definitions :: [cell_definition()]) :: {:ok, cells()}

def new(cell_definitions) do

end

New practice exercise react

4eb93dd

New practice exercise `react` New practice exercise `react` New practice exercise `react` New practice exercise `react` Change return type

angelikatyborska approved these changes Jul 7, 2021

View reviewed changes

angelikatyborska reviewed Jul 7, 2021

View reviewed changes

angelikatyborska requested a review from neenjaw July 7, 2021 07:36

jiegillet added 6 commits July 7, 2021 21:21

Add design document

45913e7

Add test with different types

9989015

Change callback to message sending function

650a34b

Contributor

c876cd2

Bug fix

5dfadb0

trailing whitespace

02e436b

neenjaw reviewed Jul 8, 2021

View reviewed changes

jiegillet added 2 commits July 8, 2021 20:14

Remove cell struct from tests

d3c1848

Contributor

968a607

angelikatyborska reviewed Jul 8, 2021

View reviewed changes

neenjaw reviewed Jul 8, 2021

View reviewed changes

Change cell type from map to tuple, rename variables

9e261dd

neenjaw approved these changes Jul 10, 2021

View reviewed changes

jiegillet added 3 commits July 10, 2021 18:29

Merge branch 'main' of github.com:exercism/elixir into jie-react

607bc10

type for cell input

d7d0a6a

I can't alphabet

4762a7e

neenjaw merged commit a29f3b0 into exercism:main Jul 10, 2021

jiegillet deleted the jie-react branch July 14, 2021 23:55

		%{type: :input, name: "input", value: 1},
		%{type: :output, name: "output", inputs: ["input"], compute: fn a -> a + 1 end}

-  @doc """
-  Start a reactive system
-  """
-  @spec new(cells :: [{:input, String.t(), any} | {:output, String.t(), [String.t()], fun()}]) ::
-          {:ok, pid}
-  def new(cells) do
-  end
+  @type input :: {:input, name :: String.t(), value :: any}
+  @type output ::
+          {:output, name :: String.t(), dependencies :: list(String.t()), callback :: function()}
+  @type cell_definition :: input() | output()
+  @opaque cells :: pid
+  @doc """
+  Start a reactive system
+  """
+  @spec new(cell_definitions :: [cell_definition()]) :: {:ok, cells()}
+  def new(cell_definitions) do
+  end

New practice exercise react #820

New practice exercise react #820

Conversation

jiegillet commented Jul 6, 2021

github-actions bot commented Jul 6, 2021

angelikatyborska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelikatyborska Jul 7, 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

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

angelikatyborska Jul 7, 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

neenjaw left a comment • edited Loading

Choose a reason for hiding this comment

jiegillet commented Jul 8, 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

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

jiegillet Jul 8, 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 Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

neenjaw Jul 8, 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

angelikatyborska commented Jul 9, 2021

neenjaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

New practice exercise `react` #820

New practice exercise `react` #820

angelikatyborska Jul 7, 2021 •

edited

Loading

angelikatyborska Jul 7, 2021 •

edited

Loading

neenjaw left a comment •

edited

Loading

jiegillet Jul 8, 2021 •

edited

Loading

neenjaw Jul 8, 2021 •

edited

Loading

neenjaw Jul 8, 2021 •

edited

Loading