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

Dot support for localField #603

Merged
merged 2 commits into from
Mar 16, 2020

Conversation

superqwer
Copy link
Contributor

@superqwer superqwer commented Mar 10, 2020

related issue #595

@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #603 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #603   +/-   ##
========================================
  Coverage    94.69%   94.70%           
========================================
  Files           16       16           
  Lines         3187     3190    +3     
========================================
+ Hits          3018     3021    +3     
  Misses         169      169           
Impacted Files Coverage Δ
mongomock/aggregate.py 96.89% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 768ec70...57d0f18. Read the comment docs.

@superqwer superqwer changed the title Allow dot support for local field Dot support for local_field Mar 11, 2020
@superqwer superqwer changed the title Dot support for local_field Allow dot support for localField Mar 11, 2020
@superqwer superqwer changed the title Allow dot support for localField Dot support for localField Mar 11, 2020
Copy link
Member

@pcorpet pcorpet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR

@@ -601,6 +601,29 @@ def fixed_getter(doc):
return fixed_getter


def _get_local_field_val(dct, path):
Copy link
Member

Choose a reason for hiding this comment

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

Please look at helpers.get_value_by_dot and consider using it. Also please add tests for some corner cases covered by get_value_by_dot not covered by your code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to get_value_by_dot
done

self.assertIn(
"Although '.' is valid in the 'localField' and 'as' parameters",
str(err.exception))
self.db.a.insert_many([
Copy link
Member

Choose a reason for hiding this comment

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

Please add an equivalent test in test__mongomock to ensure that the feature is behaving the same as in pymongo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@superqwer
Copy link
Contributor Author

Changed the resolver to get_value_by_dot
Added mongomock test

@superqwer superqwer requested a review from pcorpet March 11, 2020 10:36
@pcorpet pcorpet merged commit 9b9a343 into mongomock:develop Mar 16, 2020
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.

None yet

2 participants