-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
✨ Implement arbitrary for ulid #4020
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4d82303:
|
72e62cc
to
3d82849
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just did a quick pass on the PR from my mobile and it looks good. I'll just re-read it before approving. Thank you so much for this contribution.
Codecov Report
@@ Coverage Diff @@
## main #4020 +/- ##
==========================================
- Coverage 94.61% 94.60% -0.01%
==========================================
Files 209 211 +2
Lines 5848 5893 +45
Branches 1344 1351 +7
==========================================
+ Hits 5533 5575 +42
- Misses 299 302 +3
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@dubzzz would you like me to address the formatting/linting issues and the requirement to declare the version bump? I may need some guidance with the latter. |
For the first one, you have to run: yarn format For the bumps (declare minor on fast-check and mark all others as unchanged): yarn version:bump |
3d82849
to
7bac901
Compare
Thanks for the help, @dubzzz. I believe I addressed all the outstanding issues now 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two minor comments. We should also add the doc part for ulid and the no regression e2e test
packages/fast-check/test/unit/arbitrary/_internals/mappers/UintToBase32String.spec.ts
Outdated
Show resolved
Hide resolved
packages/fast-check/src/arbitrary/_internals/mappers/UintToBase32String.ts
Outdated
Show resolved
Hide resolved
7bac901
to
6bc3395
Compare
@dubzzz I've generated the jest snapshot covering ulids for the regression tests, and added some web documentation, as well. Let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect. Thanks for the contribution. I'll just recheck one last time from a computer but it seems perfect
c638209
to
5cdda2a
Compare
@dubzzz I had to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect 👍
Will be merged in a few minutes. Just waiting the tests to run. I'll try to release it soon |
Implements #4009
Adds arbitrary for
ulid
, according to the ulid spec.Category:
Potential impacts: