Skip to content

Conversation

blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 30, 2024

Notes for reviewer:

  • You can ignore the json files and the synchronous/* file changes

@blink1073 blink1073 marked this pull request as ready for review October 14, 2024 12:19
@blink1073 blink1073 requested review from ShaneHarvey and removed request for caseyclements October 14, 2024 12:29
:return: - An instance of :class:`~pymongo.results.UpdateResult`.

.. versionchanged:: 4.10
Copy link
Member

Choose a reason for hiding this comment

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

4.10 -> 4.11 here and elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -144,6 +145,7 @@ def add_update(
collation: Optional[Mapping[str, Any]] = None,
array_filters: Optional[list[Mapping[str, Any]]] = None,
hint: Union[str, dict[str, Any], None] = None,
sort: Optional[Mapping[str, Any]] = None,
Copy link
Member

Choose a reason for hiding this comment

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

This is slightly inconsistent with pymongo's other sort APIs which use _Sort type and allow a list of key pairs as well as a mapping. I think that's okay because mapping should be the preferred approach now (after PYTHON-2878).

I say we keep it like this and only add support for a list of pairs if a user requests it. It will save us the work of adding tests for the (list or pairs) behavior.

test/utils.py Outdated
arguments[arg_name] = list(sort_dict.items())
if spec["name"] not in ["replaceOne", "updateOne"]:
sort_dict = arguments[arg_name]
arguments[arg_name] = list(sort_dict.items())
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this code altogether because we implemented https://jira.mongodb.org/browse/PYTHON-2878 in 4.6.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -1176,8 +1188,12 @@ async def replace_one(
aggregate expression context (e.g. "$$var").
:param comment: A user-provided comment to attach to this
command.
:param sort: Specify which document the operation updates if the query matches
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a mention for these new apis in the changelog?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@blink1073 blink1073 requested a review from ShaneHarvey October 16, 2024 02:17
@@ -19,6 +19,8 @@ PyMongo 4.11 brings a number of changes including:
until either connection succeeds or a server selection timeout error is raised.
- Added :func:`repr` support to :class:`pymongo.operations.IndexModel`.
- Added :func:`repr` support to :class:`pymongo.operations.SearchIndexModel`.
- Added ``sort`` parameter to
:meth:`~pymongo.collection.Collection.update_one` and :meth:`~pymongo.collection.Collection.replace_one`
Copy link
Member

Choose a reason for hiding this comment

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

Also the bulk UpdateOne/ReplaceOne operations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@blink1073 blink1073 requested a review from ShaneHarvey October 16, 2024 18:47
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

LGTM!

@blink1073 blink1073 merged commit 6f4258c into mongodb:master Oct 16, 2024
26 of 27 checks passed
@blink1073 blink1073 deleted the PYTHON-4576 branch October 16, 2024 21:41
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