-
-
Notifications
You must be signed in to change notification settings - Fork 400
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
[#793] New practice exercise word-search
#804
Conversation
Thank you for contributing to Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:
Automated comment created by PR Commenter 🤖. |
1.7 is failing. Any guess? |
I have all the minor Elixir versions installed with |
This is what I get with Elixir 1.7 and Erlang 23.0. I'm unable to install Erlang 22 or 21 because they're too old for my system version:
|
I think @neenjaw did we want to drop 1.7 if it starts go cause trouble? |
If this is a good way to solve it, this may be a good case to prompt us to drop 1.7 support. I haven't reviewed the example solution. |
Is there a way to add an error track for the CI more detailed than Pass/Fail? I don't know much about this. I'm using |
output = WordSearch.search(grid, words) | ||
|
||
expected = %{ | ||
"clojure" => %Location{from: {10, 1}, to: {10, 7}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should make the coordinates more explicit like this:
"clojure" => %Location{from: {10, 1}, to: {10, 7}} | |
"clojure" => %Location{from: %{row: 10, column: 1}, to: %{row: 10, column: 7}} |
When I first saw {10, 1}
, I thought: {x, y}
, which is basically the opposite of what it really is 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝🏻 agreed, it isn't intuitive from a pair of numbers to know what they mean if not {x, y}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's quite interesting, to me {row, column}
is by far the most intuitive. I guess it's because I'm from a physics/math background and that's how you manipulate matrices.
Originally the tests included explicitly row and column, I'll put them back in. Sorry about that!
defmodule WordSearch do | ||
defmodule Location do | ||
defstruct [:from, :to] | ||
@type t :: %Location{from: {integer, integer}, to: {integer, integer}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secondary to that comment in the test file, we should either standardize this to {x, y}
or find a way to communicate an alternative meaning to the tuple pair
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious for your thoughts how we might be able to ease the expectation of knowing the interpretation of that tuple-pair, otherwise looks great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect to me now 🥳 @neenjaw?
As promised :)