I don't think that what you're doing is good style. I think you're adding a lot of extra complexity and places for things to go wrong, or become hard to understand, or whatever. I don't think you lose anything by just making very small methods that call super
and your special method. I talk about those options at the bottom of the post.
With that out of the way, here is (what I propose) a better and safer way tothink you can accomplish your desired goal as written and currently designed much more safely by using (againdescriptors and metaclasses. I don't think this is as good as the methods at the bottom of the post, my realbut to review the code as written here is to just implement ~15 methods that each have two lines)what I would say.
Despite being personally proud of coming up with that, I still feel pretty strongly that its the wrong way to go. As such, I wanted to show how easy this would be. For example, if you inherit from deque
:
class FileMirroredDeque(deque):
# _update_file and __init__ unchanged
def __delitem__(self, idx):
super().__delitem__(idx)
self._update_file()
# Same for each method
If you don't actually want inheritance, or want to deal with multiple inheritance, then you could just use composition instead, like so:
class FileMirroredDeque:
def __init__(self, cache_path, maxlen=None, clean=False, file_indent=None):
# Only addition to __init__
self.data = deque((), maxlen)
# Any references to self with the intent of hitting the deque should now reference self.data
def __delitem__(self, idx):
self.data.__delitem__(idx)
self._update_file()
# Same for each method
We can actually take that a step further then, by creating a generic descriptor that handles file mirroring any arbitrary collection, like so:
class FileMirroredCollection:
# This is virtually identical to your previous __init__
def __init__(self, collection, cache_path, clean=False, file_indent=None):
self.collection = collection
self.path = cache_path
self._indent = file_indent
self._bak_file = None
if clean:
self._update_file() # Overwrite contents of file.
return # Don't import data from file.
with open(self.path, 'a+') as f: # Creates file if it doesn't exist
f.seek(0, 0) # Seek back to beginning of file for json decode.
contents = f.read()
try:
# You'd need to have some intelligent way to extend the arbitrary collection; duck typing is your friend here
self.collection.extend(json.loads(contents or "[]")) # initializes internal list with persistent data or empty list.
except json.decoder.JSONDecodeError:
new_file = os.path.basename(self.path) \
+ datetime.datetime.utcnow().strftime("_%Y_%m_%d_%H_%M_%S") + ".bak"
new_path = os.path.join(os.path.dirname(self.path), new_file)
logger.warning(f"File: {self.path} was not valid JSON. It will be copied to {new_path} and a new "
f"file will be created.")
shutil.copyfile(self.path, new_path)
self._bak_file = new_path
def _update_file(self):
with open(self.path, 'w') as f:
json.dump(list(self.collection), f, indent=self._indent)
def __get__(self, obj, objtype):
self._update_file()
return self.collection
def __set__(self, obj, value):
self.collection = value
self._update_file()
def __delete__(self, obj):
del self.collection
self._update_file()
class FileMirroredDeque:
def __init__(self, cache_path, clean=False, file_indent=None, max_len=None):
self.data = FileMirroredCollection(deque((), max_len), cache_path, clean, file_indent)
# other stuff
This still needs a bit more work (e.g. this doesn't handle self.data.append
very well), but you can see how it might be useful to separate your concerns.
Lastly, there are a few other miscellaneous improvements that might benefit you, including:
- Support for more formats - json is nice, but what if I want to serialize it another way?
- Not writing to file every time; this will be inefficient if you have lots of small changes. Potentially include a
flush
method, or some way to identify that you need to write any recent changes. - As I allude to with the generic descriptor approach, supporting other data structures could be good - you might not want a deque forever, and as-is you're pretty locked down.