-
Notifications
You must be signed in to change notification settings - Fork 77
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
Refactors mongodb persister a little #472
Conversation
This is so that we can more easily test the persister, but allowing one to inject a custom client. The old way prevented that. This now then makes the behavior inline with the other persisters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to 59907ad in 53 seconds
More details
- Looked at
157
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. burr/integrations/persisters/b_mongodb.py:44
- Draft comment:
Consider handling the default assignment ofserde_kwargs
in the__init__
method to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:50%
Thefrom_values
method inMongoDBBasePersister
uses a default value forserde_kwargs
asNone
and then assigns an empty dictionary if it isNone
. This pattern is repeated in the__init__
method. It would be more efficient to handle this default assignment in one place, preferably in the__init__
method, to avoid redundancy.
2. burr/integrations/persisters/b_mongodb.py:45
- Draft comment:
Consider handling the default assignment ofmongo_client_kwargs
in the__init__
method to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:50%
Thefrom_values
method inMongoDBBasePersister
uses a default value formongo_client_kwargs
asNone
and then assigns an empty dictionary if it isNone
. This pattern is repeated in the__init__
method ofMongoDBPersister
. It would be more efficient to handle this default assignment in one place, preferably in the__init__
method, to avoid redundancy.
Workflow ID: wflow_fQAvXc4PO3Ai249v
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
A preview of 1d26f53 is uploaded and can be seen here: ✨ https://burr.dagworks.io/pull/472 ✨ Changes may take a few minutes to propagate. Since this is a preview of production, content with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 1d26f53 in 12 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_0lYwCtw0m5QkOfIS
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -35,3 +35,14 @@ def test_list_app_ids(mongodb_persister): | |||
def test_load_nonexistent_key(mongodb_persister): | |||
state_data = mongodb_persister.load("pk", "nonexistent_key") | |||
assert state_data is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so that we can more easily test the persister, but allowing one to inject a custom client. The old way prevented that. This now then makes the behavior inline with the other persisters.
Changes
How I tested this
Notes
Checklist
Important
Refactors MongoDB persister to allow custom client injection, introduces
MongoDBBasePersister
, and deprecatesMongoDBPersister
.MongoDBBasePersister
inb_mongodb.py
for custom MongoDB client injection.MongoDBPersister
, maintaining backward compatibility.persister.rst
to referenceMongoDBBasePersister
.test_b_mongodb.py
to useMongoDBBasePersister
.MongoDBPersister
.This description was created by for 1d26f53. It will automatically update as commits are pushed.