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

rnd.range(1,1) fails #3

Closed
Jerakin opened this issue Feb 2, 2021 · 6 comments
Closed

rnd.range(1,1) fails #3

Jerakin opened this issue Feb 2, 2021 · 6 comments

Comments

@Jerakin
Copy link

Jerakin commented Feb 2, 2021

No description provided.

@selimanac
Copy link
Owner

As expected!
There is no number between min: 1 and max: 1, so what is the actual problem?

@Jerakin
Copy link
Author

Jerakin commented Feb 2, 2021

I would expect it to return 1. A random number between 1 and 1 is 1. The same way a random number between 1 and 2 is 1 or 2.

Most random generators I have used (mainly python) have had the following behaviour.

rnd.range(1, 1) -> 1 always
rnd.range(1, 0) -> ValueError

# Meaning that
if min == max:
   return min

if min > max:
  return nil

The change to return nil is a breaking change :)

@selimanac
Copy link
Owner

I'll implement this, but my opinion is this not a good practice.
Trying to generate random number by using same value is pointless.
You can simply check your min and max by using if as you wrote.

if min == max:
return min

if min > max:
return nil

@Jerakin
Copy link
Author

Jerakin commented Feb 2, 2021

It's for convenience, we noticed this because in our project we use rnd to pick a random element from a table, the table always includes at least one.

With the previous behavior we had implemented this as return myTable[rnd.range(1, #myTable)]. We have have now changed that to do.

if #myTable == 1:
  return myTable[1]
else:
  return myTable[rnd.range(1, #myTable)]

Which works, even though it's a lot uglier. I am fine with either but the breaking change caught us of guard.

selimanac added a commit that referenced this issue Feb 16, 2021
@selimanac
Copy link
Owner

@Jerakin I was terribly busy for a couple of weeks, sorry.
I add this, could you please test it?

@selimanac
Copy link
Owner

@Jerakin I'm closing this for now. If you are having a trouble, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants