Skip to main content
added 4359 characters in body
Source Link
Dan Oberlam
  • 7.9k
  • 2
  • 32
  • 74

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:

  1. Support for more formats - json is nice, but what if I want to serialize it another way?
  2. 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.
  3. 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.

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.

With that out of the way, here is (what I propose) a better and safer way to accomplish your desired goal as written (again, my real review is to just implement ~15 methods that each have two lines).

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, I think you can accomplish your goal as written and currently designed much more safely by using descriptors and metaclasses. I don't think this is as good as the methods at the bottom of the post, but to review the code as written here is 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:

  1. Support for more formats - json is nice, but what if I want to serialize it another way?
  2. 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.
  3. 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.
Spelling and grammar
Source Link
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

With that out of the way, here is (what I propose) is a better and safer way to accomplish your desired goal as written (again, my real review is to just implement ~15 methods that each have two lines).

For yours, what you need is a descriptor that auto-calls super for you. Note - itsit's important to use super to handle cases of multiple-inheritance; as pointed out in the comments, your current implementation will only ever call the deque method directly, instead of taking the time to deal with __mro__. In the implementation below, we:

If that meets your needneeds, then you're done.

ItsIt's pretty tedious and error-prone to do this by hand for each method you want to apply this to. Wouldn't it be nice if there was a way to just tie into the creation of a class and have it do this for us?

There is, and the answer is metaclasses. Metaclasses are a really complex, and really powerful, tool in the Python toolbox. Anytime you find yourself using them you should think really, really hard about ifwhether that is the best way to do it. I don't think this is one of those times (see start of this answer), but if you wanted to do that, I think we'd have to do something like this:

  1. Define a way to identify the elements that should be overridden at class initialization.
  2. Define a way to identify what extra behavior needs to be added to them.
  3. Add the behavior.

In the __init__ method (__new__ would probably work too, but I ended up getting this to work as __init__ so I left it), we get the "supercharger" method to append to our functions, and our list of "superchargees" that need the behavior added to them. From there, its just a matter of making the relevant attribute look to our descriptor instead of whatever (if anything) was there before.

With that out of the way, here is (what I propose) is a better and safer way to accomplish your desired goal as written (again, my real review is to just implement ~15 methods that each have two lines).

For yours, what you need is a descriptor that auto-calls super for you. Note - its important to use super to handle cases of multiple-inheritance; as pointed out in the comments, your current implementation will only ever call the deque method directly, instead of taking the time to deal with __mro__. In the implementation below, we:

If that meets your need then you're done.

Its pretty tedious and error-prone to do this by hand for each method you want to apply this to. Wouldn't it be nice if there was a way to just tie into the creation of a class and have it do this for us?

There is, and the answer is metaclasses. Metaclasses are a really complex, and really powerful, tool in the Python toolbox. Anytime you find yourself using them you should think really, really hard about if that is the best way to do it. I don't think this is one of those times (see start of this answer), but if you wanted to do that, I think we'd have to do something like this:

  1. Define a way to identify the elements that should be overridden at class initialization
  2. Define a way to identify what extra behavior needs to be added to them
  3. Add the behavior.

In the __init__ method (__new__ would probably work too, but I ended up getting this to work as __init__ so I left it, we get the "supercharger" method to append to our functions, and our list of "superchargees" that need the behavior added to them. From there, its just a matter of making the relevant attribute look to our descriptor instead of whatever (if anything) was there before.

With that out of the way, here is (what I propose) a better and safer way to accomplish your desired goal as written (again, my real review is to just implement ~15 methods that each have two lines).

For yours, what you need is a descriptor that auto-calls super for you. Note - it's important to use super to handle cases of multiple-inheritance; as pointed out in the comments, your current implementation will only ever call the deque method directly, instead of taking the time to deal with __mro__. In the implementation below, we:

If that meets your needs, then you're done.

It's pretty tedious and error-prone to do this by hand for each method you want to apply this to. Wouldn't it be nice if there was a way to just tie into the creation of a class and have it do this for us?

There is, and the answer is metaclasses. Metaclasses are a really complex, and really powerful, tool in the Python toolbox. Anytime you find yourself using them you should think really, really whether that is the best way to do it. I don't think this is one of those times (see start of this answer), but if you wanted to do that, I think we'd have to do something like this:

  1. Define a way to identify the elements that should be overridden at class initialization.
  2. Define a way to identify what extra behavior needs to be added to them.
  3. Add the behavior.

In the __init__ method (__new__ would probably work too, but I ended up getting this to work as __init__ so I left it), we get the "supercharger" method to append to our functions, and our list of "superchargees" that need the behavior added to them. From there, its just a matter of making the relevant attribute look to our descriptor instead of whatever (if anything) was there before.

added 306 characters in body
Source Link
Dan Oberlam
  • 7.9k
  • 2
  • 32
  • 74

Note - if there were a way to safely identify the underlying data structure being modified when any of these operations is called, then that is what you should put the descriptor on (as that's what you actually want). Because I don't know of a safe way to do that, I suggest the approach below.

Note - if there were a way to safely identify the underlying data structure being modified when any of these operations is called, then that is what you should put the descriptor on (as that's what you actually want). Because I don't know of a safe way to do that, I suggest the approach below.

added 117 characters in body
Source Link
Dan Oberlam
  • 7.9k
  • 2
  • 32
  • 74
Loading
Source Link
Dan Oberlam
  • 7.9k
  • 2
  • 32
  • 74
Loading