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

feat: Add MongoDB ObjectId generation #616

Merged
merged 10 commits into from
Apr 6, 2022
Merged

feat: Add MongoDB ObjectId generation #616

merged 10 commits into from
Apr 6, 2022

Conversation

nhammond101
Copy link
Contributor

Adds a function to generate MongoDB ObjectIds

The function aims to be deliberately simple so any combination of a-z and 0-9 is generated as a valid ObjectId, instead of implementing ObjectIds according to the official spec

Futhermore, I was unsure where to add the function given it's a datatype for a database, so happy to refactor/move if required.

@nhammond101 nhammond101 requested a review from a team as a code owner March 12, 2022 16:31
@Shinigami92 Shinigami92 added the p: 1-normal Nothing urgent label Mar 12, 2022
@Shinigami92 Shinigami92 added this to the v6.2 - New small features milestone Mar 12, 2022
@Shinigami92
Copy link
Member

Would this fit better into /src/database.ts? I think we should move it to that module.

@import-brain import-brain added the c: feature Request for new feature label Mar 12, 2022
@Shinigami92
Copy link
Member

Not related to this PR, but IMO we should try to prepare that the datatype module only defines functions for typeof "return" values.
So only string, number, boolean and so on. But this would be something for v7+

@nhammond101
Copy link
Contributor Author

@Shinigami92 agree on the moving to database, and have done so now.

Is there anything I can add to the current PR to facilitate the future typeof improvements?

src/database.ts Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member

Is there anything I can add to the current PR to facilitate the future typeof improvements?

Mh, currently I don't think so. You can always join our discord if you haven't already, so we can communicate better there.
But for now, we would tackle all the v6.1 stuff first anyways.
We expect to release 6.0.0 on monday evening.

@codecov
Copy link

codecov bot commented Mar 12, 2022

Codecov Report

Merging #616 (1dff5dc) into main (5beac4b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 1dff5dc differs from pull request most recent head 892576e. Consider uploading reports for the commit 892576e to get more accurate results

@@           Coverage Diff           @@
##             main     #616   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files        1921     1921           
  Lines      176656   176667   +11     
  Branches      905      906    +1     
=======================================
+ Hits       175503   175514   +11     
  Misses       1097     1097           
  Partials       56       56           
Impacted Files Coverage Δ
src/database.ts 100.00% <100.00%> (ø)

Nick Hammond added 2 commits March 12, 2022 17:43
Signed-off-by: Nick Hammond <[email protected]>
src/database.ts Outdated Show resolved Hide resolved
src/database.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Mar 12, 2022
@ST-DDT ST-DDT requested a review from a team March 12, 2022 20:00
@Shinigami92 Shinigami92 dismissed their stale review March 12, 2022 20:23

Looks good for now, will approve after v6.1 released

@damienwebdev
Copy link
Member

This feels more like a plugin than a method of the main library. @Shinigami92 @ST-DDT have you seen a way to add custom user-defined methods to the Faker API?

@ST-DDT
Copy link
Member

ST-DDT commented Mar 13, 2022

have you seen a way to add custom user-defined methods to the Faker API?

This is possible, but it requires casting or subclassing the faker instance. While I agree this kind of custom/specific, moving this to a plugin seems overengeneered to me. We can consider this again, when we have more of these.
We could nest these in their own submodule if you want.
faker.database.mongodb.objectId()

@Shinigami92
Copy link
Member

I'm with @ST-DDT, plugins would be over engineered.
We also already had the discussion about sub module, but IMO we can move it into that if really needed.

src/database.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested review from a team April 5, 2022 22:37
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Apr 5, 2022
src/database.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Apr 5, 2022
@ST-DDT ST-DDT requested a review from a team April 5, 2022 22:40
pkuczynski
pkuczynski previously approved these changes Apr 5, 2022
import-brain
import-brain previously approved these changes Apr 6, 2022
src/database.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested a review from a team April 6, 2022 07:49
@Shinigami92 Shinigami92 enabled auto-merge (squash) April 6, 2022 07:57
@Shinigami92 Shinigami92 merged commit a5b3888 into faker-js:main Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants