16 replies to this topic

[Huevos]

First a hack...

Then another hack...

Then this...

https://github.com/O...6a6543c3310bc7b

Then this...

https://github.com/O...b05c4757ce495cb

 

Then a new plan. Get the red paint, pour it into the blue paint and stir it with a big stick so no one can figure out where the Superclass ends and the subclass begins..

https://github.com/O...ead1206dfc65b76

https://github.com/O...d532dbb026c7b3c

 

Code is now spaghetti. Not even a comment to explain why a new method from a specific module has be placed in a generic module.

 

And all that was needed was a simple tweak in Recordtimer.py. Not one change in Timer.py is necessary.

 

Whatever happened to OpenPLi quality control?

 

[WanWizard]

As you can see in some of these PR's, I was against it from the start, and I still am. I can't comment on why Littlesat merged it.

[Huevos]

I need to test this code but I think the whole thing  could have been handled like this in the sub-class without any other changes.

 

https://github.com/H...e08d3afd716b6de

[WanWizard]

I think part of the issue is that the inheritance is misused / misunderstood in this case.

 

Both classes are subclasses from Timer, but one subclass requires access to a method in another subclass. That smells of bad design, either that method is in the wrong place, or the wrong class is subclassed.

[Taapat]

You have missed a comment in the first commit on which you are pointing.

ims change goal is save timers in file on cleanup.

 

My commits are trying to create it without Class methods overridden. By the way overridden is not a hack as you write, but normal practice in python.

 

I think this all indicates that the cleanup function is wrong from the start, but it is hard to change it a lot, as much else uses it.

[Huevos]

Taapat if you look at my commit you will see how that is done from the subclass.

 

And you are not overriding. You are just adding methods from the sub-class into the superclass. The code is now all mixed together. Why not just get all the code from enigma and put it all in one big super.super-class.

Edited by Huevos, 22 January 2019 - 13:25.

[Huevos]

I think part of the issue is that the inheritance is misused / misunderstood in this case.

I think you are right. But also massive changes when a small tweak is needed, followed by bug fixes to fix all the BSoDs that the first change caused. And the end result is spaghetti. Needs to be reverted.

Edited by Huevos, 22 January 2019 - 13:28.

[WanWizard]

I agree. I've pinged Littlesat, it's his merge.

[Taapat]

Taapat if you look at my commit you will see how that is done from the subclass.

 

And you are not overriding. You are just adding methods from the sub-class into the superclass. The code is now all mixed together. Why not just get all the code from enigma and put it all in one big super.super-class.

 

Your commit is exactly what I immediately suggested in the comment in the second commit on which you point  https://github.com/O...omment-31806009

But then this didn't like anyone.

Edited by Taapat, 22 January 2019 - 14:07.

[Taapat]

I think that if the method is used in superclass, then it also must be defined in superclass. And this is my commit point.

 

Another question is or it should be used in superclass.

[Huevos]

 

Taapat if you look at my commit you will see how that is done from the subclass.

 

And you are not overriding. You are just adding methods from the sub-class into the superclass. The code is now all mixed together. Why not just get all the code from enigma and put it all in one big super.super-class.

 

Your commit is exactly what I immediately suggested in the comment in the second commit on which you point  https://github.com/O...omment-31806009

But then this didn't like anyone.

 

It is the correct way so that the super class does not get contaminated with code from the subclass.

[Huevos]

 

I think that if the method is used in superclass, then it also must be defined in superclass. And this is my commit point.

 

Another question is or it should be used in superclass.

 

It shouldn't be used in the super class unless it is desired behaviour for every subclass that uses the super class.

 

But anyway these commits cause other knock on effects such as this one:

https://github.com/O...ry.py#L522-L523

[Taapat]

There is also an unwanted side effect in plugin epgrefresh, so I also think that it would be better to use timer.Timer.cleanup() in RecordTimer.

 

But I have a question why in class Timer should be exactly cleanup but not for example delete or save?

Edited by Taapat, 22 January 2019 - 14:30.

[Huevos]

@Tapaat. I have tested my code and it works fine so far. So I think the last 2 commits to recordtimer.py should be reverted and then use my commit.

 

No idea about the naming of the methods but that is a different issue and so should be handled with a different commit if it is deemed necessary.

 

But please remember when renaming methods (A.K.A. fiddling) that you break any plugin or other code that uses those methods.

Edited by Huevos, 22 January 2019 - 14:41.

[Huevos]

 

Taapat if you look at my commit you will see how that is done from the subclass.

 

And you are not overriding. You are just adding methods from the sub-class into the superclass. The code is now all mixed together. Why not just get all the code from enigma and put it all in one big super.super-class.

 

Your commit is exactly what I immediately suggested in the comment in the second commit on which you point  https://github.com/O...omment-31806009

But then this didn't like anyone.

 

Tapaat, I didn't see that until you pointed it out. No idea why you felt it was a hack. It is the correct way to extend a method in Python. But seeing as we have both come up with the identical (simple) solution independently, I am now certain it is the correct one.

Edited by Huevos, 22 January 2019 - 14:50.

[littlesat]

Please revert the whole thing -or- at least arrange it get right... Huevos approach is smarter as it targets the real issue.... The orriginal way had uneffect side effects which were work-a-round and then the spaghetti begins... Sometimes this (uuh shit) happens...

Edited by littlesat, 22 January 2019 - 14:53.

[Taapat]

Tapaat, I didn't see that until you pointed it out. No idea why you felt it was a hack. It is the correct way to extend a method in Python.

Because in both cases are used overridden.

Here in sub class RecordTimer overridde method saveTimer from super class Timer: https://github.com/O...b05c4757ce495cb

Here in sub class RecordTimer overridde methodes cleanup and cleanupDaily from super class Timer: https://github.com/O...omment-31806009

Edited by Taapat, 23 January 2019 - 07:45.