-
-
Notifications
You must be signed in to change notification settings - Fork 525
high-scores: Regenerate tests #936
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
Conversation
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'd prefer not to make changes now. For several reasons, one of them #935 , and also I don't want to confuse mentors with having 3 different versions, now that we're going to change the report method shortly too.
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.
Thanks so much Pedro!
I wonder: should we wait with merging until the Mentor Note update is merged? 🐔 🥚
Edit: or merge the notes pr as well on merging this. 🐣
|
||
"Your latest score was #{latest}. That's #{difference} your personal best!".squeeze | ||
def personal_top_three | ||
scores.max(3) |
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 think I'd prefer to have the 'minimal solution for approval', not the 'best'.
(Because it seems that some mentors refer to the example, or at least look at it.)
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.
This shouldn't be too relevant 😕 These solutions are not for mentor consumption - and even if a mentor does see this I don't see it as a big issue at all. I'd rather keep things fairly separate - mentor notes are one thing, these solutions are another.
What do you think? Do you feel strongly about this? I can revert the change, but I don't want us to start changing these solutions every time we change the mentor notes - unless that's something we decide to do across all tracks.
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.
Not very strongly, but let's say: preferrable. Strangely exactly because I agree with you: that we shouldn't start changing these solutions. Unless it's a bad example/ bad practice / outdated practice.
AFAIK the current line is none of those, or is it?
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.
It's not. I'll change it back.
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.
But really, the only reason I didn't commit max(3)
the first time around is because I didn't know about it 😅
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.
It's not. I'll change it back.
Actually, scores.sort.reverse.take(3)
does do a bit of extra work, when compared to max(3)
.
Also, my point about not starting to change these solutions was specifically about changing them every time we change the mentor notes. I think we should be free to change these as we please, without the mentor notes having much influence. My main point is that (at least for now) they are different things and should change independently, if needed.
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.
Ooooh, this was absolutely not meant to be blocking merging this PR! I'm sorry.
Let's try to merge them at the same time. |
Update tests to:
3.0.0 999ec47
4.0.0 ad1f9c4
These are the changes introduced by:
v3.0.0 notes
Right now I kept the new method name
personal_top_three
, but a case could be made for naming itpersonal_top_3
or ratherpersonal_top3
, as the rubocop styleguide suggests. Personally, I'm not a fan ofpersonal_top3
🤔 Thoughts are welcome.@F3PiX I believe we can make this change without affecting the overhauling efforts, but let me know if that's not the case.
v4.0.0 notes
Also updated README.md 👍
TODO
report
method.