exercism / elixir-analyzer

GNU Affero General Public License v3.0
30 stars 32 forks source link

Don't care about `case` in new-passport; require `with` with `else` #324

Closed angelikatyborska closed 2 years ago

angelikatyborska commented 2 years ago

Resolves https://github.com/exercism/elixir-analyzer/issues/323

As long as with was used, the purpose of the exercise was fulfilled. There are perfectly idiomatic solutions that use case in a helper, for example:

def get_new_passport(now, birthday, form) do
  with {:ok, timestamp} <- enter_building(now),
       {:ok, manual} <- validate_no_coffee_break(now),
       counter = manual.(birthday),
       {:ok, checksum} <- stamp_form(timestamp, counter, form),
       number <- get_new_passport_number(timestamp, counter, checksum) do
    {:ok, number}
  end
end

defp validate_no_coffee_break(now) do
  case find_counter_information(now) do
    {:coffee_break, _} -> {:retry, now |> NaiveDateTime.add(15 * 60)}
    ok -> ok
  end
end
jiegillet commented 2 years ago

I think removing this check is not enough, we should replace it with one that checks that else is being used (simple enough with a feature I think). Our purpose is to teach about with and else, so as long as the message is clear about that goal we should keep doing that (and I think it's pretty clear the way it is). I know the person that opened the ticket suggested otherwise, but we should still nudge the students towards learning new things before they can make better informed decisions.

angelikatyborska commented 2 years ago

Added a check for else: 9747a86 (#324) but it will only work if with/else is used directly in the desired function. I think if people added a helper function there, it would be a mistake by itself (too much boilerplate), so I'm fine with that drawback.

Website copy PR: https://github.com/exercism/website-copy/pull/2219