-
Notifications
You must be signed in to change notification settings - Fork 1
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
Matcha groups size #42
base: master
Are you sure you want to change the base?
Conversation
src/utils/Logger.ts
Outdated
@@ -25,7 +25,7 @@ export default createLogger({ | |||
format: json(), | |||
transports: [ | |||
new Transports.Console({ | |||
level: 'debug', | |||
level: 'error', |
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.
delete
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 see the vision! Some comments here:
src/commands/Matcha.ts
Outdated
@@ -104,17 +107,31 @@ export default class Matcha extends Command { | |||
const shuffledMembersList = Matcha.shuffle(users); | |||
const memberPairings = []; | |||
|
|||
const groupsize = interaction.options.getInteger('groupsize', true); | |||
Logger.error(`/matcha - ready to match ${shuffledMembersList}`); |
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.
why is this Logger.error here? Is this a logging statement we can remove?
src/commands/Matcha.ts
Outdated
} | ||
memberPairings.push(pairedMembers); | ||
} | ||
|
||
Logger.error(`/matcha - matching worked? ${memberPairings}`); |
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.
again here, i don't think we need this logging anymore ^
src/commands/Matcha.ts
Outdated
if (groupsize <= 1) { | ||
await super.edit(interaction, { | ||
content: 'You need to enter an integer greater than 1 otherwise your only friend will be yourself :angry:', | ||
components: [], | ||
ephemeral: true, | ||
}); | ||
return; | ||
} |
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.
Nit: I think we can rewrite this error message to be more in line with what our other messages look like
Instead of integer, we can say "group size", and also add some punctuation
**/matcha** needs a group size greater than 1! Otherwise, your only friend will be yourself! :rage:
src/commands/Matcha.ts
Outdated
const extraMembers = shuffledMembersList.length % groupsize; | ||
const numGroups = Math.floor(shuffledMembersList.length / groupsize); | ||
Logger.error(`/matcha - numGroups ${numGroups}, extraMembers: ${extraMembers} groupsize ${groupsize}`); | ||
if (extraMembers !== 0) { | ||
// If length % size is off add people to the first group or first n groups | ||
// for one group remaining add all extras | ||
if (numGroups === 1) { | ||
pairedMembers = shuffledMembersList.splice(0, shuffledMembersList.length); | ||
Logger.error(`/matcha - pair ${pairedMembers}`); | ||
} | ||
// otherwise, disperse extra people across groupss | ||
else { | ||
const addedToGroup = Math.floor(extraMembers / numGroups); | ||
pairedMembers = shuffledMembersList.splice(0, groupsize + addedToGroup); | ||
} |
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.
Nit: I think we can make this code more clear by breaking the code out into a helper function (maybe splitIntoGroups()
?)
Additionally, I think we need to consider edge cases here:
- What if the group size is 3 and we have 5 total members? I think this code makes a pair of 4 and leaves the last person lonely
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.
Looks good to merge! Let's resolve the conflict in yarn.lock then merge + deploy
added a field to the matcha command for group size. The groupsize is minimum group size. If there is not a clean split, it will add across multiple groups. This can lead to larger groups than intended.