Skip to content
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

!random with numbers doesn't work #396

Closed
zuzak opened this issue Jan 13, 2020 · 2 comments · Fixed by #399 or #401
Closed

!random with numbers doesn't work #396

zuzak opened this issue Jan 13, 2020 · 2 comments · Fixed by #399 or #401
Labels
bug problems that existing features cause

Comments

@zuzak
Copy link
Contributor

zuzak commented Jan 13, 2020

You should be able to do !random 5 but the bot doesn't output anything if you try.

I think this is because the function in question outputs a Number: https://github.com/zuzakistan/civilservant/blob/master/modules/random.js#L8

whereas the output formatter doesn't support it: https://github.com/zuzakistan/civilservant/blob/master/modules/_commands.js#L6-L21

To fix this bug, either:

  • change the output formatter to correctly handle numbers (harder), or
  • coerce the output of the random command's function to be a string (easier)
@zuzak zuzak added the bug problems that existing features cause label Jan 13, 2020
georgewatson pushed a commit that referenced this issue Jan 13, 2020
Fixes #394
Fixes #311
Fixes #310
Fixes #396
zuzak pushed a commit that referenced this issue Jan 13, 2020
(not 100% confident in this fix)
@zuzak zuzak mentioned this issue Jan 13, 2020
zuzak pushed a commit that referenced this issue Jan 13, 2020
(not 100% confident in this fix)
@zuzak
Copy link
Contributor Author

zuzak commented Jan 13, 2020

Looks like this doesn't work.
We've accidentally applied the fix to the wrong part of the !random command: the part that picks a random choice from a list (!random foo bar baz) as opposed to a random number (!random 5).

We're converting a string to a string, which wasn't broken.
The problem is with the second return statement; the first one was fine.

Sorry about this; I should've caught this during code review.

@zuzak zuzak reopened this Jan 13, 2020
@G-Thompson-Ops
Copy link
Contributor

G-Thompson-Ops commented Jan 14, 2020

I need help understanding the logic here @zuzak - Is this statement saying in real terms if the message is two words long (!random 12) : else if (msg.args.length === 2) then do some maths then return an int (which is this case is res)?

In this case we just need to change to res.toString in order to solve this issue - although I still don't fully understand the else logic, nor how the math actually works (Where in the below statement it uses the number we specified?)

var res = Math.floor(Math.random() * Math.floor(msg.args[1]))

Is the logic here saying return output of ([random number between 0-1] * the second 'word' (number) from the !random 12 command) rounded up to the nearest whole number?

zuzak pushed a commit that referenced this issue Jan 14, 2020
* Update random.js

Fixed #396 - Removed pointless conversion and var and added conversion to var 'res' - This is because the handler can't interpreit var's with properties other than string.

This is a greater problem and should probably be re-visitied.

* Update random.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug problems that existing features cause
Projects
None yet
2 participants