MartinHeroux / spike2py

spike2py provides a simple interface to analyse and visualise data
GNU General Public License v3.0
12 stars 8 forks source link

Major update to codebase #25

Closed MartinHeroux closed 4 years ago

MartinHeroux commented 4 years ago

As you will see I have updated things based on our recent conversation (chaining and mixins).

  1. While the mixins works, I don't see how I will be able to test it in isolation given that many of the methods call self.values but the mixins class does not actually have this attribute. do I need to have an init in my mixins class? So that I can pass values that can be set to self.values?

  2. Also fixed tests based on revised codebase (they all pass).

bbelderbos commented 4 years ago

@MartinHeroux that is a great question!

https://coderbook.com/@marcus/deep-dive-into-python-mixins-and-multiple-inheritance/ "A mixin is not meant to be used by itself." - so no, don't add a constructor and having this "magic" self.values is ok, because the class that this is "mixed in" will provide that.

"You want to use one particular feature in a lot of different classes." - this to me confirms (again) we're using the appropriate design pattern here.

Also interesting how he compares it to decorators:

A Mixin might be used to add a new set of methods to a class, instead of just modifying behavior it adds new blocks of code and new features to the existing class.

Also, in my opinion, Mixins are nicer to use for composing functionality of a new class. Theoretically, you could decorate a new class to compose it with functionality, but having a list of 5 decorators that wrap each other, is more confusing and difficult to understand than to use Mixins that compose the new behavior.

bbelderbos commented 4 years ago

For testing, nothing stops you from setting values manually in your tests right? e.g. mixin_instance.values = ... - let me know if that works for you ...

(Pdb) sp
<__main__.SignalProcessing object at 0x7fbf14aafc50>
(Pdb) sp.rect
<bound method SignalProcessing.rect of <__main__.SignalProcessing object at 0x7fbf14aafc50>>
(Pdb) sp.rect()
*** AttributeError: 'SignalProcessing' object has no attribute 'values'
(Pdb) sp.values = {}
(Pdb) sp.rect()
*** TypeError: bad operand type for abs(): 'dict'
(Pdb) sp.values = 1
(Pdb) sp.rect()
<__main__.SignalProcessing object at 0x7fbf14aafc50>
(Pdb)
bbelderbos commented 4 years ago

Did the values assignment for the mixin class work for you when writing tests? Did not see that yet here in the PR.

Edit: nws I read here you are working on this.

MartinHeroux commented 4 years ago

Adding a self.values will definitely work for the mixin. I just need to implement it! It is my next task.

bbelderbos commented 4 years ago

OK cool let me know if you get stuck or anything.