Add analyzer for high scores exercise #111
Conversation
|
Hello. Thanks for opening a PR on Exercism 🙂 We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in. You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch. If you're interested in learning more about this auto-responder, please read this blog post. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
| } | ||
| end | ||
|
|
||
| def approve_if_whitespace_is_sensible(msg = nil, params = {}) |
There was a problem hiding this comment.
I removed the bang operator from these methods when I moved them into the parent class because the ruby style guide defines the bang operator a way to identify a dangerous version of an equivalent method without the bang operator.
There was a problem hiding this comment.
Thank you, and yes, exactly that. The ! on a method communicates that there is a safe method of that name as well, and it can be confusing when that is not the case.
There was a problem hiding this comment.
On this same line (and just as a knee jerk reaction to this) the params probably is something that should not be used very often, as we do have keyword arguments. Having keyword arguments means that they get documented better, otherwise you have to go look to find out what the "parameters" might be that are valid.
There was a problem hiding this comment.
This is likely a concern for a different patch though.
There was a problem hiding this comment.
There's a lot that needs to be cleaned up, such as dead code and unused test files.
| attr_reader :scores | ||
| def initialize(scores) | ||
| @scores = scores |
There was a problem hiding this comment.
Consider bring strict in our attribute use. It will let us know more directly if/when a mistake happens here.
There was a problem hiding this comment.
I don't follow. Could you be more specific?
There was a problem hiding this comment.
It's OK, more a reference to me later or if someone wants to set up attribute use as advice for the students.
|
@iHiD I do not have enough permission to reopen in this repository. |
|
Could someone reopen this? |
Give it a bit of time, Jeremy is extremely busy. This is not a priority PR. |
|
Thanks. I can't reopen this as it was force pushed. It wil need to be recreated please. |
|
I recreated it here |
3 participants
Closes #13
Closes #10