Proper abstraction of Module store base class implemented

[Slater-Victoroff]

@dmitchell @cpennington Just some small changes to make the inheritance structure clearer, and cleaning out a couple unused inputs.

[dmitchell]

Given that you left in the update methods, why did you remove the delete method?

[Slater-Victoroff]

Largely because python apparently doesn't support conditional abstract methods (currently applying a decorator counts as overriding the method) and delete_item was the only method that isn't current in use by every concrete class that inherits from this one. I could also remove the other update methods and put them in a separate class though.

[cpennington]

If you're trying to be fully correct about this, your best bet is probably to slice the modulestore api into the Read and Write components. XML would implement only the Read portion, and mongo and split would implement both.

I'm not sure what you mean about 'applying the decorator counts as overriding the method'. From the rest of your comment, it seems like the issue is more that not all classes provide an implementation of delete_item, and if it's marked with @abstractmethod then any subclass that doesn't override it also has to be abstract.

[Slater-Victoroff]

Ah sorry, that was related to an earlier conversation. An original thought was to dynamically apply the abstractmethod decorator depending on the class to actually be instantiated, but I think dividing it into Read and Write components makes more sense.

[dmitchell]

Cale plans to refactor the modulestore api. At a minimum, we need to get rid of the update_xxx variants. I don't see this as in any way impeding that refactoring tho. 👍

[cpennington]

Why not leave in this default implementation? It's possibly slow, but correct.

[dmitchell]

I'm not sure it will work w/ split (doesn't use i4x nor Location). Regardless, doesn't the concrete impl violate the class as an abstract interface class?

[Slater-Victoroff]

My thought was that this is an abstract class and as such it should be a completely abstract class. It was also overriden everywhere it was actually used.

[cpennington]

Fair point about not working with split. Abstract classes don't have to be devoid of implementation, though (unlike Java). You can have an abstract class (can't be instantiated) with concrete methods (providing default functionality).