Skip to content

GODRIVER-2119 implement methods required to use as a map key #715

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

Merged
merged 3 commits into from
Aug 18, 2021

Conversation

Luberry
Copy link
Contributor

@Luberry Luberry commented Aug 6, 2021

GODRIVER-2119

Implements the TextMarshaler and TextUnmarshaler methods on primitive.ObjectID to allow it to be used as a map key.

Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution @Luberry ! Just some small comments.

@Luberry Luberry requested a review from benjirewis August 12, 2021 20:51
Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

LGTM! I'll wait for the rest of the team to review; thanks.

Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @Luberry! I have a question about an alternative for UnmarshalText.

@kevinAlbs kevinAlbs self-requested a review August 13, 2021 18:46
Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

Approved, with a few suggestions, and pending resolution of the UnmarshalText conversation.

require.Equal(t, expectedJSON, out)
}

func TestObjectID_UnmarshalJSONMap(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider incorporating this test function as a test case in TestObjectID_UnmarshalJSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not add it in TestObjectID_UnmarshalJSON because i did not see a clean way of slotting it into the table tests.

Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM, thank you again for the contribution @Luberry!

@benjirewis benjirewis merged commit 1b281b8 into mongodb:master Aug 18, 2021
faem pushed a commit to kubedb/mongo-go-driver that referenced this pull request Mar 17, 2022
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