revert recent changes to ConfigfromDB subs #1527
Conversation
|
Unfortunately this moves backwards, not forwards. So I'm not happy about that. See #1528 |
|
The move backwards was intentional. Right now the priority is to get stable code so we can release. You've not given us a good reason why you made this change in the first place, other than you have a preference to have it in a different module. I don't see that as a particularly good reason to go changing code, which has worked fine for literally a decade, especially when we are trying to get a release out the door. If you've got a good reason to do this, then let's do it after 1.30.0 release, and in a separate branch so we can safely test it, along with all the other changes we backed out recently. |
|
How about stop reverting bits and pieces of my code that were tested and were approved and were applied! Your PR removed the code, but didn't fix the calls which specify the Config module instead of the ConfigAdmin module. Stop dicking around with my code. I'm trying to get this release out too, but just reverting code without understanding the effect is not useful. Let's all try to understand the problem, and move forward. I'm not approving your PR and I don't expect you to approve mine, so we are going to need some other people to come back from wherever they went to. |
|
Both have valid points, and since I approved the PR's being reverted I guess I better get involved. It was working so why change? It was part of a larger move to clean up perl modules. Am I correct that originally stemmed from the db connection being made in multiple places, since reverted? As it appears now, the benefit is one less module to build, and the issue was never caused by this change itself but a reversion of DB changes. But I hear @knnniggett concerns about changing something that works now. So looking at that we have moved the functions without change to a new location and updated how they are called. |
|
My position is just that I like to move forward rather than backwards. Reverting has similar dangers. I believe in understanding the problem, and fixing it correctly, rather than just putting it off to another day. I believe that my PR fixes the problem, and leaves us with better code. So I am advocating that. I was ok with the initial reversing, thinking it would simplify things, but it hasn't. It has made things more complex. I feel that we should have investigated, understood and fixed the problem instead. Too late now. |
|
No. It doesn't. Your treatment of me since you've gotten back has been bad. I don't accept that it has anything to do with ZM. I missed you so much while you were away. It has been really unpleasant since you've been back though. I look forward to our development moving forward. To that end, as I have said, I suggest we try to understand the problem, and fix it. Instead of just reverting. that is my position, I bow to the will of the democracy. |
|
Jump on voice and hug it out gents, it is a simple difference of opinions. No ones changes are dicking with the code, they are just trying to solve problems, and happen to have a different approach to your own. @knnniggett this PR doesn't fully revert yet, https://github.com/ZoneMinder/ZoneMinder/blob/master/scripts/zmupdate.pl.in#L343. |
|
FYI the related error from above when installing this PR against 1.29 |
|
#1528 solved it, I merged it. |
This fixes the warnings about the xxConfigFromDB functions being redefined.