Class Inheritance

[PabloRuizCuevas]

I'm checking at the code, maybe to help a bit and provide it with some type hints in the near future.

Just wanted to raise that I don't fully understand the porpoise of the add_methods() and _add_operations() functions, why don't we use inheritance, or add them as methods inside the same class? Maybe I'm missing something, but for me, I find the usage of this function makes the code a bit hard to read-debug-improve.

For instance layering layer takes "self", but this is a function where self should be a class instance of stairs and as such the name self is confusing. Either layer is a method of stairs with the argument "self" or layers is separate function that takes a instance of stairs with "stairs_instance" (whatever name) instead of "self".

Of course I may be missing something, but I have never seen this pattern so far. Thanks anyway :)

Maybe useful [link]:(https://stackoverflow.com/questions/533631/what-is-a-mixin-and-why-is-it-useful) for the case.

[venaturum]

Hey Pablo, once upon a time the Stairs class was completely contained within a single module.

see version 1 for instance https://github.com/staircase-dev/staircase/blob/v1.0.0/staircase/stairs.py

Version 2 saw a large refactoring and introduction of a lot of new functionality. I definitely prefer organising the functionality into separate modules, eg staircase.core.ops.arithmetic for addition, subtraction, and staircase.core.ops.logical for "and" and "or" etc, and this is inspired by the structure of the pandas library. I particularly like that this allows the functionality to be broken into smaller methods, which don't need to pollute the Stairs class, or be imported into the stairs.py module. Could this have been achieved with mixins? Yeh I think so, and semantically the two approaches aren't too different.

The use of the add_methods and _add_operations functions was partly inspired by pandas,

see https://github.com/pandas-dev/pandas/blob/3f310c450d879cc4b85825e235616bf5076f0584/pandas/core/ops/methods.py#L122

We do differ in that they will often define a function somewhere, such as pandas.core.reshape.melt which does all the heavy lifting for melting a dataframe, and then they define DataFrame.melt where they simply call this melt function, as opposed to stitching it on, .

If I remember correctly, the use of add_methods (as opposed to stitching functions on with setattr directly in the stairs.py module was to get around circular imports. Now, if I was to change the parameter name away from self in these functions, then that would show in the documentation, which is something I'd rather not change - for all intents and purposes I'd like it to appear as any other method on the class. And if they weren't intended to be methods on the class then they are way too coupled with Stairs to stay in their current form and a major refactor would be on the cards.

With regards to "Either layer is a method of stairs with the argument "self" or layers is separate function". I'm not sure I think of it as black and white as this. For example given the following definitions

a = pd.Series(1) 
b = pd.Series(2)
func = pd.Series.add

the following are all equivalent

a.add(b)
pd.Series.add(a,b)
func(a,b)

i.e. they literally are the same function

> len(set([f.__code__.co_code for f in [a.add, pd.Series.add, func]]))
1

yet func doesn't belong to any object of type Series. So for me the definitions of methods of a class, and functions are blurred (with respect to Python - a language like java is a different kettle of fish).

Hopefully that explains what was going through my head at the time. Certainly there are negatives. If I run dir(staircase.Stairs) for example then I can see the class has a method called add. This method will appear in VS Code intellisense for Stairs methods, yet if I type staircase.Stairs.add I don't get an option to "go to definition" like I do with staircase.Stairs.diff which is defined directly in the Stairs class. If I had my time again I'd probably try with mixins, and maybe it would solve that problem. There also remains the argument that I could be using the pandas approach of defining and delegating in equivalent methods, but I'm sure I'd run into circular imports - which could be a code smell of a bad design itself.

[PabloRuizCuevas]

Yes, now I see why you did it, but as you said there are many negative aspects related to "intellisense", code autocompletion and static type hints, mypy will surely have lots of complains. On the other hand IDEs like Pycharm raise a warning "attribute not in class", in default the configuration. For me the combination of these issues are really bad.

On the other hand, I still need to check with some more time the pandas approach that you provided, but somehow they manage to avoid any sort of issue like the ones I described before. So if you can adapt your current approach to avoid these annoyances would be great :)

I think we could make a refactor on this, that will have bidirectional positive impact with the type hints I'm implementing etc, unfortunately I'm not familiar enough with the code to do such a big change, but I could help you testing different architectures and giving ideas.