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

fix: make inspect for ObjectId work #412

Merged
merged 1 commit into from
Nov 13, 2020
Merged

Conversation

addaleax
Copy link
Contributor

@addaleax addaleax commented Nov 5, 2020

The previous Object.defineProperty() calls did not work because
they did not specify a property descriptor, and even if they had
worked, the signatures of .inspect() and .toString() would
have mismatched, and even if they weren’t, the result would not
have been very useful as debugging output, because it only returned
the raw hex content of the ObjectId and not even e.g. the fact
that this object is an ObjectId.

I’m happy with any other solution, but this particular one would
make my life a tiny bit easier :)

Description

Make the inspect and [util.inspect.custom] methods on ObjectId work.


Not sure if I should open a JIRA ticket for this, but happy to do so if it helps :)

The previous `Object.defineProperty()` calls did not work because
they did not specify a property descriptor, and even if they had
worked, the signatures of `.inspect()` and `.toString()` would
have mismatched, and even if they weren’t, the result would not
have been very useful as debugging output, because it only returned
the raw hex content of the `ObjectId` and not even e.g. the fact
that this object is an `ObjectId`.

I’m happy with any other solution, but this particular one would
make my life a tiny bit easier :)
@mbroadst
Copy link
Member

@addaleax thanks! Yes, can you please open a JIRA ticket a well? That will help us make better release notes

@addaleax
Copy link
Contributor Author

@mbroadst Neal already did that for me :) https://jira.mongodb.org/browse/NODE-2875

I wasn’t sure whether that was necessary/helpful but I’ll open tickets for everything in the future 👍

Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@addaleax
Copy link
Contributor Author

@mbroadst Not sure if your re-approval is a hint for me to merge this, because if so, I have to tell you that I can’t :)

@mbroadst mbroadst requested review from nbbeeken and emadum November 13, 2020 14:48
@mbroadst
Copy link
Member

@addaleax thanks for the bump. I actually want to make sure we have at least two approvals from the team before changes are merged, we'll take care of merging for you

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@nbbeeken nbbeeken merged commit a585a0c into mongodb:master Nov 13, 2020
@addaleax addaleax deleted the fix-inspect branch November 13, 2020 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants