Add spirit flakes option to fish command #5948
Conversation
|
Do you have a test bot that you tried this on? |
|
Yeah I created a fresh server which I used as a sandbox and testing ground for the bot. |
src/mahoji/commands/fish.ts
Outdated
|
|
||
| // Remove the bait from their bank. | ||
| await user.removeItemsFromBank(new Bank().add(fish.bait, quantity)); | ||
| if (shouldRemoveFromBank) { |
There was a problem hiding this comment.
Unless im misunderstanding, this variable isnt needed, you can just do if (cost.length > 0) to check if there any items in the cost bank.
There was a problem hiding this comment.
You're right, I fixed that.
src/lib/util/smallUtils.ts
Outdated
| export function pluraliseItemNameWithQuantity(name: string, quantity: number): string { | ||
| const result = `${quantity} ${name}`; | ||
| if (quantity === 1) return result; | ||
| return pluraliseItemName(result); | ||
| } |
There was a problem hiding this comment.
This function seems odd to me, if its only 1, it will include the quantity, but if its anything more than 1, it wont show the quantity? I generally prefer now to just write: 1x Coal, 1x Egg, 5x Trout, this way theres no pluralization needed.
There was a problem hiding this comment.
I was hoping that the unit tests written for this method would make its functionality clear, but I can try to elaborate: Even if quantity != 1, it will still be shown. Consider name is "pike" and quantity is 5.
result==="5 pike"pluraliseItemName("5 pike")is called- return "5 pike" + "s" =>
"5 pikes"
I added this method because I noticed the 1x item format wasn't consistently used. Few examples I have on hand are the output of
k(Your minion finished killing 553 Man. Your Man KC is now 553.),mine(Your minion finished mining 327 Pure essence. You received 1,635 :mining: XP. (3k/Hr)) andfish(Your minion finished fishing 24 Sardine. You received 480 :fishing: XP. (18k/Hr))
It would facilitate correct English if someone wants to run those outputs through pluraliseItemNameWithQuanity, as a future refactoring.
I can change the output of fish to not use this function. Though, after reading this, do you think this function is still useful? Or should I remove it?
There was a problem hiding this comment.
I think using the 1x format is better and we should stick with that
There was a problem hiding this comment.
And the function itself? Delete?
Description:
Spirit flakes did not have any use beyond the cosmetic change to the angler outfit. This PR aims to bring the functionality where they can be used to provide a 50% chance for double catch during fishing.
Changes:
Added an optional boolean
flakesparameter to thefishcommand. Provided you have at least one spirit flake in your bank, they will be used to increase the amount of fish caught. If you have fewer flakes than the provided quantity, all your flakes will be used and the remaining fish will not provide an increased yield. Does not increase XP gains.Other checks: