Added == #14
Added == #14
Conversation
This reverts commit 8991055.
Pull Request Test Coverage Report for Build 105
💛 - Coveralls |
|
Thanks for this @MattLParker! There's a few things I'd like to change before getting this into master, primarily fixing a bit of duplicate code and updating some inline documentation. I know you've already put a lot of effort into this; are you interested in some detailed feedback or would you prefer I make the changes and go from there? |
|
Hey, We are using the code on a slack group already as is. I'm really just trying to dig into JS, So feel free to make changes and I will watch and learn :) |
|
No probs! I'll aim to get to it this weekend :) Thanks again for contributing the code back! |
There was a problem hiding this comment.
General review comments:
- You didn't note
==in the help message in thesendHelp()function inevents.js - We really need a way to do site-specific customisations, e.g. messages, help text, leaderboard emoji, etc.
- On tone of the messages: I'm trying to keep them positive and supportive, "How is Frank currently at 143 points" doesn't really work within those goals. (Also you're missing a question mark) - again this is a site-specific thing which we really need to support better (and I really need to get off my ass and sort that out)
- I was going to comment on the table creation stuff in the score lookup, however that's just copy-pasted from elsewhere, so 🤷♂️
The only thing I need sorted pre-approval is the lower case g and removing the site-specific customisations you've made. (You're going to have to carry those changes yourself for the time being)
src/points.js
Outdated
| * @param {string} operation The mathematical operation performed on the item's score. | ||
| * @return {int} The item's new score after the update has been applied. | ||
| */ | ||
| const GetScore = async( item ) => { |
There was a problem hiding this comment.
Lower case g to match the style of the other methods.
| }, | ||
| { | ||
| probability: 1, | ||
| set: [ ':shifty:' ] |
There was a problem hiding this comment.
I see you've also joined the cult of the shifty. One of us! One of us! One of us!
|
Oh, and on tone, I'm happy with the messages for now, I'll work out better wording for us and change that separately. |
|
Oh, and you should update |
|
Our main repo for our slack is https://github.com/winadminsdotorg/PointsBot |
|
I think that covers those problems? |
SkUrRiEr
left a comment
SkUrRiEr
left a comment
There was a problem hiding this comment.
Thanks, looks good to me!
90% of the "problems" with this PR is the site-specific stuff. Implementing that properly is next on my list for development (once I get back to this project). Please feel free to send us more PRs! |
5 participants
Added == and made it where bot can get points as well. @bot == will throw error in log but does work.