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

MONGOID-5670 [Monkey Patch Removal] Remove Hash#delete_id and Hash#extract_id #5701

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Sep 2, 2023

Fixes MONGOID-5670

Moved to Nested::NestedBuildable

These should first be marked deprecated, so I will raise a PR to 8.1 branch.

Also it seems there are no specs for these. Since they are private, it will stay that way.

Overall progress is tracked here: http://tinyurl.com/mongoid-monkey. Refer to MONGOID-5660 for context.

@johnnyshields johnnyshields changed the title Move Hash#delete_id and Hash#extract_id to Nested::NestedBuildable MONGOID-5670 [Monkey Patch Removal] Remove Hash#delete_id and Hash#extract_id Sep 3, 2023
@johnnyshields johnnyshields marked this pull request as draft September 3, 2023 18:12
@johnnyshields johnnyshields changed the title MONGOID-5670 [Monkey Patch Removal] Remove Hash#delete_id and Hash#extract_id WIP - MONGOID-5670 [Monkey Patch Removal] Remove Hash#delete_id and Hash#extract_id Sep 3, 2023
@@ -160,7 +160,7 @@ def embedded?
#
# @return [ Object ] The id.
def extract_id
selector.extract_id
selector['_id'] || selector[:_id] || selector['id'] || selector[:id]
Copy link
Contributor Author

@johnnyshields johnnyshields Sep 3, 2023

Choose a reason for hiding this comment

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

The usage of extract_id for a query Criteria selector seems to be different than usage for extracting from a document Hash. For example, the code comment for this method says Could be in an $and query, which it would seem the current logic does not cover.

I think it makes sense to inline the code here, given the query vs. document usage difference, but would like to hear thoughts.

Also, I think for query selector we may only need to consider the string case. TBD

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind seeing this inlined here, even though it does seem to duplicate the logic. It's better than the monkey patch, for sure. Also, yeah, this probably does only need to consider the string case, but I'm in favor of leaving it as-is simply because that's the literal implementation that existed before. 👍

@johnnyshields johnnyshields changed the title WIP - MONGOID-5670 [Monkey Patch Removal] Remove Hash#delete_id and Hash#extract_id MONGOID-5670 [Monkey Patch Removal] Remove Hash#delete_id and Hash#extract_id Sep 3, 2023
@johnnyshields johnnyshields marked this pull request as ready for review September 3, 2023 18:22
@jamis jamis merged commit 7378141 into mongodb:master Nov 7, 2023
16 of 17 checks passed
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.

2 participants