Add analyzer for high scores exercise #113
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. |
|
I re-opened this per #111 (comment) |
You might want to stick around a bit for this, I do not have access at this level to approve or open, but I can close. So it is a "mix of permissions" concerns here at a minimum. Some one that does not have the rights to reopen or approve affectively probably should not have rights to close. |
SleeplessByte
left a comment
SleeplessByte
left a comment
There was a problem hiding this comment.
@kotp here you go, guardian sign-off.
kotp
left a comment
kotp
left a comment
There was a problem hiding this comment.
Slight changes to consider, but looks good.
| } | ||
| end | ||
|
|
||
| def approve_if_whitespace_is_sensible(msg = nil, params = {}) |
There was a problem hiding this comment.
The msg seems to be the comment, when I am testing this. Later we use {comment: msg, params:} and if the names are the same this can be reduced to {comment:, params:}.
This happens in two places in this code.
There was a problem hiding this comment.
I feel that this is outside of the scope of this PR since this is not technically new code. I was just moving it.
There was a problem hiding this comment.
I would like to do another PR to clean up some things up, so I could make this change then if you like.
There was a problem hiding this comment.
In some ways this is an "Add analyzer" so making it exemplar now provides a good template for new things coming in. But of course, it does not matter in the grand scheme of things.
| end | ||
| end | ||
|
|
||
| def disapprove(msg, params = {}) |
There was a problem hiding this comment.
Here, as mentioned, is the second place the msg happens when it is likely comment.
| # can probably be extracted to parent | ||
| # ### | ||
|
|
||
| def approve_if_implicit_return!(msg = nil, params = {}) |
There was a problem hiding this comment.
If the code is moved, then it is ripe for change as well. Same argument (would rather do it here than have a PR that might not come in). If you are good with that.
PatrickMcSweeny
force-pushed
the
high-scores
branch
from
78b1a64 to
432dcf1
Compare
PatrickMcSweeny
force-pushed
the
high-scores
branch
from
432dcf1 to
83ed9f4
Compare
|
I brought it in, adding a commit to the history about the side effect where TwoFer was affected. (I generally do research using git rather than github, as it is faster, so it makes sense to me to have the "why" in the commits directly.) |
|
The website-copy PR still needs to be merged. |
3 participants
Closes #10
Closes #13