Add analyzer for gotta-snatch-em-all#451
Conversation
kahgoh
force-pushed
the
exercise/gotta-snatch-em-all
branch
from
f9544bc to
7260296
Compare
|
I notice is failing, but I think its because the comments aren't yet in the website-copy. Is there anything I need to do? |
|
No, there's nothing more that you should do - you have already created the PR with the comments 🙂 when we review this PR, we will first merge the analyzer comments, then rerun the CI to see it all turn green. I'm currently swamped with life stuff so I was hoping @jiegillet can find the time to review this. |
|
I'm also a bit tight these days, but I'll have time soon, I'll get to this :) |
There was a problem hiding this comment.
Sorry, this isn't an in-depth review yet, and it looks good from a bird's eye view, but we usually add tests for new rules.
The idea for the tests is not to test the mechanics of the analyzer, but to "justify" or explain the rules. If a rule is "don't use X", we commit a test with a solution where X is used, and we can see how the code is worse. If the solution with X is actually better, or if there is no way of finding a solution with X, then it means that the rule itself is not actually helping.
Each test doesn't need to include the full solution, if a rule is about function f, just including the module with f defined is fine.
|
No worries @jiegillet 😉. Thanks for taking a look! Double checking my understanding in regards to the tests - are the tests like the ones in test/elixir_analyzer/test_suite? |
|
Yes, exactly! |
jiegillet
left a comment
jiegillet
left a comment
There was a problem hiding this comment.
This looks great, thank you so much.
I really liked your test examples, they really demonstrate the breadth of Elixir, there are so many ways to get the same result and many of the ones you used are completely valid. I think we need to emphasize in the comments that the solutions we recommend are not the only ones, they are just using the concepts we want to teach in this exercise.
Ok, for the next steps:
- I'll review your website-copy PR
- we get it merged
- we re-run the tests in this PR (to make sure we have the right paths to the comments)
- we merge this PR
Co-authored-by: Jie <jie.gillet@gmail.com>
|
Fantastic. Thank you again for the contribution and your patience with us, I know it took a while. |
3 participants
Analyzer comments in exercism/website-copy#2387
Closes #450