Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

The array extension method :sample throw an argument error when the array is empty #94

Closed
pake007 opened this issue Nov 30, 2012 · 5 comments

Comments

@pake007
Copy link

pake007 commented Nov 30, 2012

Hi, stympy

I am using the newest version of faker, seems the extension of array method :sample breaks the default ruby :sample when the array is empty and there gives an argument.

  • ruby default :sample
    [].sample(3) => []
  • faker extension :sample
    [].sample(3) => wrong number of arguments (1 for 0)

Though this example seems a little wired, I really meet this error in my project. Please fix it.

@pake007
Copy link
Author

pake007 commented Nov 30, 2012

tip: I am using ruby 1.8.7

@pake007
Copy link
Author

pake007 commented Dec 3, 2012

Sorry, the first example is not ruby, should be rails

@michaelkrenz
Copy link

IMHO this is not really a bug. Ruby 1.8.7 does not have the Array#sample method (it was introduced in 1.9). The project's authors chose to implement a backport of that method, which was incomplete, but good enough for their purposes, as it was always used without arguments here. It relies on Ruby 1.8.7's Array#choice method, which does not accept an argument, hence the error message you mentioned. So in a Ruby 1.8.7 environment, you cannot simply use Array#sample, but should use the native Array#choice method or replicate that functionality using Kernel#rand.

On the other hand, as there is already a backport for this method in the project, I think it would be a worthwhile addition to improve it and make it fully compatible with its Ruby 1.9 counterpart. So I made a patch and wrote some corresponding tests. All tests ran successfully under Ruby 1.8.7 and 1.9.3.

The patch code is based on the excellent Backports project (https://github.com/marcandre/backports) - no need to reinvent the wheel here. I simplified it a bit to keep it manageable (I didn't want to replicate all of their Backport-internal features).

@pake007
Copy link
Author

pake007 commented Dec 3, 2012

Thanks @michaelkrenz, really rapid patch!
Btw, backports is excellent, starred it!

@antillas21
Copy link

@pake007 could you please close this issue? as @michaelkrenz pointed out his patch has been merged (years ago now).

@pake007 pake007 closed this as completed Sep 7, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants