Checking that the store is an instance of dict seem incorrect. #636
Conversation
Carreau
force-pushed
the
memorystore-is-not-dict
branch
from
b339f56 to
c7e6ce3
Compare
Codecov Report
@@ Coverage Diff @@
## master #636 +/- ##
==========================================
+ Coverage 99.45% 99.94% +0.48%
==========================================
Files 28 28
Lines 10287 10288 +1
==========================================
+ Hits 10231 10282 +51
+ Misses 56 6 -50
|
Indeed MemoryStore is an instance of mutable mapping that have the same property as dict, but woudl not be seen as as instance of dict.
Carreau
force-pushed
the
memorystore-is-not-dict
branch
from
676754a to
4a39c97
Compare
| @@ -1955,7 +1956,7 @@ def _encode_chunk(self, chunk): | |||
| cdata = chunk | |||
|
|
|||
| # ensure in-memory data is immutable and easy to compare | |||
| if isinstance(self.chunk_store, dict): | |||
| if isinstance(self.chunk_store, MutableMapping): | |||
There was a problem hiding this comment.
As mentioned upthread, this change is too broad and may force unnecessary copies. The goal of this check was to make sure there would not be unexpected mutation of in-memory data so would copy to bytes before storing. If we are writing to disk or sending to a cloud store, this is unneeded. Perhaps the check needs to be updated, but maybe that can be handled as part of Greg's work?
|
Have cherry-picked the doc fixes from here into PR ( #907 ) |
|
@grlee77 : any thoughts on what to do here? |
|
Guessing Greg has likely handled this along with other things in his PRs. Though could be wrong about that. |
|
I think the same is true. Closing in favor of #898 and friends. |
Indeed MemoryStore is an instance of mutable mapping that have the same
property as dict, but would not be seen as as instance of dict.
With some caveat inlines.