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

Regression(?) in sample behavior for strings #2927

Closed
dylemma opened this issue May 12, 2021 · 2 comments · Fixed by #2944
Closed

Regression(?) in sample behavior for strings #2927

dylemma opened this issue May 12, 2021 · 2 comments · Fixed by #2944
Assignees
Labels

Comments

@dylemma
Copy link

@dylemma dylemma commented May 12, 2021

Hi and thanks for the really handy library! I've had a dependency on underscore on a work project for many years now (we were using version 1.8.3) and I just merged a dependabot update which pulled in version 1.12.1 and noticed an unexpected change in the behavior of the sample function.

Previously, you could pass a string as the list argument, and it would be treated as an array of characters, e.g.

// with underscore 1.8.3
_.sample('abcdef', 3) // returns ["c", "f", "b"]
_.sample('abcdef', 3) // returns ["b", "e", "f"]

But now it just returns a substring from the beginning of the string, with no randomness

// with underscore 1.12.1+ (also tried on underscorejs.org)
_.sample('abcdef', 3) // returns "abc" every time
_.sample('abcdef', 4) // returns "abcd" every time

Other observations:

  • if I don't pass an n argument, it does return a random letter, e.g. _.sample('abcdef') returns "d" or "a" etc
  • it works as expected if I split the string into an array of characters e.g. _.sample(['a', 'b', 'c', 'd', 'e', 'f'], 3)
@jgonggrijp
Copy link
Collaborator

@jgonggrijp jgonggrijp commented May 12, 2021

Thanks for reporting, @dylemma. I was able to trace this down to #2158 and confirm that this is indeed a regression, which was already introduced in 1.9.0 (ironically, right after the version you were using).

The reason is that in 1.8.3, sample/shuffle was filling a new empty array, while from 1.9.0 onwards, it starts with a clone of the input collection. It now uses _.clone for "array-like" objects, which unfortunately returns primitive values unmodified. Since primitive strings are immutable, this results in no values being permuted, explaining your issue. Singular samples are unaffected because they are handled in a separate branch that doesn't involve cloning.

Sorry about the inconvenience. I will try to address this soon. In the meanwhile, you can patch Underscore as follows so you don't have to edit your code everywhere:

var originalSample = _.sample;

function fixedSample(collection, n, guard) {
    if (_.isString(collection)) collection = _.toArray(collection);
    return originalSample(collection, n, guard);
}

_.mixin({sample: fixedSample});

@jgonggrijp
Copy link
Collaborator

@jgonggrijp jgonggrijp commented Dec 16, 2021

@dylemma @KenProle fixed, will be in the next release, which I expect to be up within a day or so.

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

Successfully merging a pull request may close this issue.

2 participants