Repository navigation
fix(compat/random): compatibility issues with random function - #1767
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I came across a useful tool for checking compat(skill)-> https://github.com/toss/es-toolkit/blob/main/.claude/skills/compat-review/SKILL.md Having it automatically generate a report would make it much easier for reviewers to evaluate the changes. |
| } | ||
| }); | ||
|
|
||
| it('should support floats', () => { |
There was a problem hiding this comment.
This test actually comes from lodash, so I'd rather not change it much — especially the expect assertions. Could we keep those as-is and just add more tests on top instead?
There was a problem hiding this comment.
Of course! I'll take a look over the weekend 😁
dafe6d4 to
f66f887
Compare
| expect(samples.every(v => v >= 0 && v <= max)).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should support floats', () => { |
There was a problem hiding this comment.
One thing I'm curious about: I noticed that some compat tests include links to the corresponding Lodash test cases, but many others (such as random) don't.
Is there a general convention around this?
If the goal is to keep Lodash's original test cases intact, would it make sense to split them into two describe blocks?
first describe: test cases from Lodash
second describe: additional test cases added by es-toolkit
I'm not sure why some tests have links while others don't, but it feels like we could benefit from a bit more consistency. Separating the tests might also make their purpose clearer.
(Though I admit that could turn into a somewhat tedious task... maybe manageable with a little help from AI 🤔)
There was a problem hiding this comment.
Ideally, every test should have a link pointing to its equivalent Lodash test. I think some contributors just omitted them along the way as we received contributions from many different people.
There was a problem hiding this comment.
Sounds good, I've taken a look 😁
It might take a bit of time, but I'll work on it.
While working on this, I felt it would be better to keep the two tests separated. When we revisit them in the future, it will be much easier to identify which tests originated from Lodash and which ones were added by us. I think that distinction will make the test suite much easier to understand and maintain.
Incorrect Range(
floating=true) -> b15a478Summary
Currently, when floating is true, values are returned in a range 1 higher than max.
e.g.
when min=1, max=2 with floating enabled, values like 2.xx can be returned.Reproduce
Return integer when float value in parameter -> fdf699b
Summary
In all of the above cases, integers are returned.
However, Lodash returns a float if at least one float value is included in the arguments.
REproduce
Return integer when (max=1, floating=true) case -> dafe6d4
Summary
Returns an integer when called with (max=some..., floating=true)
Shouldn't it return a float since
floating=true😁Reproduce