Skip to content

Conversation

blink1073
Copy link
Member

No description provided.

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 assuming the tests pass

@blink1073
Copy link
Member Author

I scheduled the doctests variant

@blink1073
Copy link
Member Author

The docs build now, but we are failing two of the actual doctests

@ShaneHarvey
Copy link
Member

Let's fix the two failing tests. The CodecOptions repr test starting failing after the typing change here: https://evergreen.mongodb.com/task/mongo_python_driver_tests_doctests__platform~ubuntu_18.04_python_version~3.6_doctests_52ed5a4135a76480e03e96eb0369c2c4eae0c3f7_22_01_13_22_09_48:

 [2022/02/28 20:28:11.596] Document: faq
 [2022/02/28 20:28:11.596] -------------
 [2022/02/28 20:28:11.621] **********************************************************************
 [2022/02/28 20:28:11.621] File "faq.rst", line 263, in key-order
 [2022/02/28 20:28:11.621] Failed example:
 [2022/02/28 20:28:11.621]     opts
 [2022/02/28 20:28:11.621] Expected:
 [2022/02/28 20:28:11.621]     CodecOptions(document_class=<class 'bson.son.SON'>,
 [2022/02/28 20:28:11.621]                  tz_aware=False,
 [2022/02/28 20:28:11.621]                  uuid_representation=UuidRepresentation.UNSPECIFIED,
 [2022/02/28 20:28:11.621]                  unicode_decode_error_handler='strict',
 [2022/02/28 20:28:11.621]                  tzinfo=None, type_registry=TypeRegistry(type_codecs=[],
 [2022/02/28 20:28:11.621]                                                          fallback_encoder=None))
 [2022/02/28 20:28:11.621] Got:
 [2022/02/28 20:28:11.621]     CodecOptions(document_class=bson.son.SON, tz_aware=False, uuid_representation=UuidRepresentation.UNSPECIFIED, unicode_decode_error_handler='strict', tzinfo=None, type_registry=TypeRegistry(type_codecs=[], fallback_encoder=None))

6.0-latest has added new fields to duplicate key errors, we'll need to add them or ignore them:

 [2022/02/28 20:28:12.751] -----------------------
 [2022/02/28 20:28:12.751] **********************************************************************
 [2022/02/28 20:28:12.751] File "examples/bulk.rst", line 128, in default
 [2022/02/28 20:28:12.751] Failed example:
 [2022/02/28 20:28:12.751]     try:
 [2022/02/28 20:28:12.751]         db.test.bulk_write(requests, ordered=False)
 [2022/02/28 20:28:12.751]     except BulkWriteError as bwe:
 [2022/02/28 20:28:12.751]         pprint(bwe.details)
 [2022/02/28 20:28:12.751] Expected:
 [2022/02/28 20:28:12.751]     {'nInserted': 0,
 [2022/02/28 20:28:12.751]      'nMatched': 1,
 [2022/02/28 20:28:12.751]      'nModified': 1,
 [2022/02/28 20:28:12.751]      'nRemoved': 1,
 [2022/02/28 20:28:12.751]      'nUpserted': 0,
 [2022/02/28 20:28:12.751]      'upserted': [],
 [2022/02/28 20:28:12.751]      'writeConcernErrors': [],
 [2022/02/28 20:28:12.751]      'writeErrors': [{'code': 11000,
 [2022/02/28 20:28:12.751]                       'errmsg': '...E11000...duplicate key error...',
 [2022/02/28 20:28:12.751]                       'index': 0,...
 [2022/02/28 20:28:12.751]                       'op': {'_id': 1}},
 [2022/02/28 20:28:12.751]                      {'code': 11000,
 [2022/02/28 20:28:12.751]                       'errmsg': '...E11000...duplicate key error...',
 [2022/02/28 20:28:12.752]                       'index': 2,...
 [2022/02/28 20:28:12.752]                       'op': {'_id': 3}}]}
 [2022/02/28 20:28:12.752] Got:
 [2022/02/28 20:28:12.752]     {'nInserted': 0,
 [2022/02/28 20:28:12.752]      'nMatched': 1,
 [2022/02/28 20:28:12.752]      'nModified': 1,
 [2022/02/28 20:28:12.752]      'nRemoved': 1,
 [2022/02/28 20:28:12.752]      'nUpserted': 0,
 [2022/02/28 20:28:12.752]      'upserted': [],
 [2022/02/28 20:28:12.752]      'writeConcernErrors': [],
 [2022/02/28 20:28:12.752]      'writeErrors': [{'code': 11000,
 [2022/02/28 20:28:12.752]                       'errmsg': 'E11000 duplicate key error collection: '
 [2022/02/28 20:28:12.752]                                 'bulk_example.test index: _id_ dup key: { _id: 1 }',
 [2022/02/28 20:28:12.752]                       'index': 0,
 [2022/02/28 20:28:12.752]                       'keyPattern': {'_id': 1},
 [2022/02/28 20:28:12.752]                       'keyValue': {'_id': 1},
 [2022/02/28 20:28:12.752]                       'op': {'_id': 1}},
 [2022/02/28 20:28:12.752]                      {'code': 11000,
 [2022/02/28 20:28:12.752]                       'errmsg': '',
 [2022/02/28 20:28:12.752]                       'index': 2,
 [2022/02/28 20:28:12.752]                       'keyPattern': {'_id': 1},
 [2022/02/28 20:28:12.752]                       'keyValue': {'_id': 3},
 [2022/02/28 20:28:12.752]                       'op': {'_id': 3}}]}
 [2022/02/28 20:28:12.752] **********************************************************************

@blink1073
Copy link
Member Author

Updated and rescheduled

doc/faq.rst Outdated
unicode_decode_error_handler='strict',
tzinfo=None, type_registry=TypeRegistry(type_codecs=[],
fallback_encoder=None))
CodecOptions(document_class=bson.son.SON, tz_aware=False, uuid_representation=UuidRepresentation.UNSPECIFIED, unicode_decode_error_handler='strict', tzinfo=None, type_registry=TypeRegistry(type_codecs=[], fallback_encoder=None))
Copy link
Member

Choose a reason for hiding this comment

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

Is this a bug or does it have something to do with inheriting from SON now inheriting typing.Dict? On Python 3.6:

>>> repr(SON)
'bson.son.SON'

Vs Python 3.7+

>>> repr(SON)
"<class 'bson.son.SON'>"

Should we bother trying to make this consistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

In Python 3.7+, the built in container types are generic as well. I don't think it is worth supporting the Python 3.6 syntax in the doctests.

Copy link
Member

@ShaneHarvey ShaneHarvey Mar 1, 2022

Choose a reason for hiding this comment

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

Oh I see:

>>> repr(SON)
"<class 'bson.son.SON'>"
>>> repr(SON[str, int])
'bson.son.SON[str, int]'
>>> repr(MongoClient)
"<class 'pymongo.mongo_client.MongoClient'>"
>>> repr(MongoClient[dict])
'pymongo.mongo_client.MongoClient[dict]'

Seems odd that they didn't stick with one representation. There's probably a related CPython or Mypy bugs report but I can't find it at the moment.

'index': 0,...
'op': {'_id': 1}},
{'code': 11000,
'errmsg': '...E11000...duplicate key error...',
'errmsg': '',
Copy link
Member

Choose a reason for hiding this comment

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

The server is returning an empty errmsg seems unexpected and likely a bug in v6.0-latest. Could you file a server ticket for this?

Perhaps for now we can just do '...' for the second errmsg field?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -141,11 +141,12 @@ and fourth operations succeed.
'upserted': [],
'writeConcernErrors': [],
'writeErrors': [{'code': 11000,
'errmsg': '...E11000...duplicate key error...',
'errmsg': 'E11000 duplicate key error collection: '
'bulk_example.test index: _id_ dup key: { _id: 1 }',
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 the previous version with ... is more robust to changes in the server's errmsg.

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 March 1, 2022 16:47
Copy link
Contributor

@juliusgeo juliusgeo left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -36,7 +36,7 @@ insert a couple of example locations:
>>> result = db.places.insert_many([{"loc": [2, 5]},
... {"loc": [30, 5]},
... {"loc": [1, 2]},
... {"loc": [4, 4]}]) # doctest: +ELLIPSIS
... {"loc": [4, 4]}])
Copy link
Contributor

@juliusgeo juliusgeo Mar 1, 2022

Choose a reason for hiding this comment

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

This # doctest: +ELLIPSIS feature is cool. So it's just another approach to wildcard characters basically?

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically, yep

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 782c551 into mongodb:master Mar 1, 2022
@blink1073 blink1073 deleted the PYTHON-3146 branch March 1, 2022 20:10
juliusgeo pushed a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 5, 2022
juliusgeo pushed a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 7, 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.

3 participants