-
Notifications
You must be signed in to change notification settings - Fork 47
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
Fix bug in SimStore when appending to non-existent files #941
Fix bug in SimStore when appending to non-existent files #941
Conversation
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.
1 piece of excess code in the test and 1 question about coherence of abc
between the different files inside /simstore
@@ -5,7 +5,7 @@ | |||
import logging | |||
logger = logging.getLogger(__name__) | |||
|
|||
class SimpleNamespace(collections.MutableMapping): | |||
class SimpleNamespace(collections.abc.MutableMapping): |
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.
Any reason why this file does not do a similar thing as memory_backend.py
(or the other way around)?
(Define abc
and use that)
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.
No particular reason. I don't really have a consistent style when it comes to imports. I went ahead and switched this to just use abc
. Really, I should have been using the separate abc
package for Py27, not collections.MutableMapping
. I think the previous existence of that package is why I prefer from collections import abc
(plus it is much shorter). On the other hand, I usually just do import os
when using things in os.path
, so there's not a general rule to apply.
Co-authored-by: Sander Roet <sanderroet@hotmail.com>
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.
LGTM
Pretty much what the title says. Before, there was an error if you opened a new file as append, because it tried to read the non-existent schema from the file.
Also cleans up some deprecations from importing ABCs from
collections
, notcollections.abc
(SimStore is Python 3+.)