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

Internal: refactor isinstance-dict to isinstance-MutableMapping #245

Merged
merged 1 commit into from Nov 6, 2018

Conversation

@caichinger
Copy link
Contributor

caichinger commented Oct 30, 2018

No description provided.

@caichinger caichinger force-pushed the caichinger:refactor_isinstance_dict branch 2 times, most recently from cc4b29b to 915a48d Oct 30, 2018
@msiemens

This comment has been minimized.

Copy link
Owner

msiemens commented Nov 3, 2018

Thanks for the PR, @caichinger! One small request: could you add a small test case, for example in tests/test_tinydb.py?

@caichinger caichinger force-pushed the caichinger:refactor_isinstance_dict branch from 915a48d to ef01ac1 Nov 4, 2018
@caichinger

This comment has been minimized.

Copy link
Contributor Author

caichinger commented Nov 4, 2018

@msiemens sure, I should have thought of that myself :)

I added two small tests to tests/test_tinydb.py as you suggested.

However, it occurred to me that checking for Mapping only is actually sufficient and the current version is therefore based on Mapping.

In my opinion in favor of Mapping.

  • less strict
  • sufficient

In favor of MutableMapping:

  • more in line with the dict-like (mutable) nature of the documents

Personally, I am inclined towards Mapping since the check only occurs upon insertion and at this point a Mapping (mutable or not) is needed.

What would you prefer? I would then make the necessary changes.

@eugene-eeo

This comment has been minimized.

Copy link
Contributor

eugene-eeo commented Nov 4, 2018

@caichinger just my 20c - Mapping better describes what we want from the object (i.e. that we only want to read from it). Since we do not modify the document when we generate an ID for it, this is fine.

From a backwards compatibility POV, MutableMapping might be better (since it is a relaxation of isinstance(x, dict)) since we can always relax it more or decide not to, whereas if we decide that we want isinstance(x, MutableMapping) in the future then we'd have to bump a major API version.

@msiemens

This comment has been minimized.

Copy link
Owner

msiemens commented Nov 6, 2018

I think I'm totally fine with using Mapping. Semantically it seems like the right choice as we don't really want to modify documents while they are being stored in the database.

@caichinger caichinger force-pushed the caichinger:refactor_isinstance_dict branch from ef01ac1 to dc52b5a Nov 6, 2018
@caichinger

This comment has been minimized.

Copy link
Contributor Author

caichinger commented Nov 6, 2018

@eugene-eeo @msiemens thank you two for your comments.

Please take a look at the PR and let me know if you want me to make any amendments.

@msiemens msiemens merged commit e23b831 into msiemens:master Nov 6, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@msiemens

This comment has been minimized.

Copy link
Owner

msiemens commented Nov 6, 2018

@caichinger Thanks for your contribution! 🙂

@msiemens

This comment has been minimized.

Copy link
Owner

msiemens commented Nov 6, 2018

This is now released in v3.12.0 🎉

@caichinger

This comment has been minimized.

Copy link
Contributor Author

caichinger commented Nov 6, 2018

@msiemens cool, thank you for accepting my suggestion! :)

@caichinger caichinger deleted the caichinger:refactor_isinstance_dict branch Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.