Pull Request Updates...
Pull Request Updates...
Hi PeterU,
Now that the 2016-04-19 firmware has gone official and the NSW school holidays are over will you be able to start considering outstanding pull requests?
I believe Prl has looked at the code and not had any issues to report (other that a skin question that I posted here and had no responses). I think IanB has also reviewed the code but I have not seen any comments in support or otherwise. I suspect that in this case, no news is good news.
I am hoping to see a new beta released with the latest changes so I can move forward with some improvements to OverlayHD that address some user requests and concerns.
Regards,
Ian.
Now that the 2016-04-19 firmware has gone official and the NSW school holidays are over will you be able to start considering outstanding pull requests?
I believe Prl has looked at the code and not had any issues to report (other that a skin question that I posted here and had no responses). I think IanB has also reviewed the code but I have not seen any comments in support or otherwise. I suspect that in this case, no news is good news.
I am hoping to see a new beta released with the latest changes so I can move forward with some improvements to OverlayHD that address some user requests and concerns.
Regards,
Ian.
Re: Pull Request Updates...
I'd like to get around to it soon and have a new beta going out as soon as practical. I'm currently a bit snowed under and that is unlikely to change for the next few days.
Re: Pull Request Updates...
Hi PeterU,
No problem, thanks for the update.
Regards,
Ian.
No problem, thanks for the update.
Regards,
Ian.
Re: Pull Request Updates...
Hi PeterU,
My pull request discussions have appeared in a few threads but this is the only one that deals specifically with this one issue so I will keep my updates here.
I have submitted my time and format updates for EPGSearch to the appropriate repository and the code has been accepted and merged. The changes use the date / time code changes in "Components/UsageConfig.py" when they are available and also accepts a flag from "Components/EPGList.py" to indicate that the Beyonwiz build, or any other Enigma2 that uses both start and end times in the EPG, is in use. and updates the display accordingly.
The code has been made safe for any builds that do not use either feature so it can be safely merged into the current Beyonwiz firmware.
I am working on the OpenViX versions of the other changes. I have raised a test pull request to check the process out and will start submitting the rest of the enhancements soon.
Unfortunately there will need to be some Beyonwiz only changes, like the "self.showend" value in the Beyonwiz repository to flag the Beyonwiz exclusive use of both start and end times in the EPG.
Regards,
Ian.
My pull request discussions have appeared in a few threads but this is the only one that deals specifically with this one issue so I will keep my updates here.
I have submitted my time and format updates for EPGSearch to the appropriate repository and the code has been accepted and merged. The changes use the date / time code changes in "Components/UsageConfig.py" when they are available and also accepts a flag from "Components/EPGList.py" to indicate that the Beyonwiz build, or any other Enigma2 that uses both start and end times in the EPG, is in use. and updates the display accordingly.
The code has been made safe for any builds that do not use either feature so it can be safely merged into the current Beyonwiz firmware.
I am working on the OpenViX versions of the other changes. I have raised a test pull request to check the process out and will start submitting the rest of the enhancements soon.
Unfortunately there will need to be some Beyonwiz only changes, like the "self.showend" value in the Beyonwiz repository to flag the Beyonwiz exclusive use of both start and end times in the EPG.
Regards,
Ian.
Re: Pull Request Updates...
Hi PeterU,
I have now submitted my "InputDevice.py" changes to OpenViX so it should be easier for you to merge in the future.
Are there any other changes I have previously made to the Beyonwiz repository that you would like placed in OpenViX to make your merges easier?
Regards,
Ian.
I have now submitted my "InputDevice.py" changes to OpenViX so it should be easier for you to merge in the future.
Are there any other changes I have previously made to the Beyonwiz repository that you would like placed in OpenViX to make your merges easier?
Regards,
Ian.
Re: Pull Request Updates...
This is a merge nightmare scenario. Almost the entire file has merge conflicts due to what are essentially "spinning your wheels" changes. That's because the Beyonwiz base already had PEP8 fixes applied, but it was done in such a way that it was mainly a "whitespace only" changeset. Your changes are such that automation can not resolve the conflicts.IanSav wrote:changes in "Components/UsageConfig.py"
...
I am working on the OpenViX versions of the other changes. I have raised a test pull request to check the process out and will start submitting the rest of the enhancements soon.
I also noticed you submitted some changes where you rolled both functional changes and formatting changes into a single commit. That's a big no-no. It makes it impossible to simply accept the formatting changes and then resolve the functionality conflicts. If you absolutely can not resist making frivolous code formatting changes, please keep them separate.
Difficult merges, such as this one, only lead to wasted effort and are very, very likely to introduce bugs and regressions due to merge noise. To give you an idea, merging your formatting changes to one file is more effort that merging all of the other upstream changes in the entire enigma2 code base from the last 6 months.
Re: Pull Request Updates...
Hi PeterU,
There has been only one commit accepted by OpenViX which relates to "Components/UsageConfig.py" under commit number 06a0a40. This is purely a clean up with no functional changes. This was the first accepted commit and is to test the waters of working with the OpenViX team.
I decided on a clean up only change to find the optimal way of working with the OpenViX team. This wasn't my first submission to OpenViX but it was the first of acceptable quality for them to merge.
When we find a compatible development style I hope to submit the date and time changes that I proposed and discussed here about 8 months ago. Those changes were, at the time, reviewed by others and no problems or objections were reported. Later I would like to move on to proposing other improvements and ideas.
As we are now working on the Beyonwiz firmware that is more closely based on OpenViX you had encouraged me to directly contribute to the OpenViX build. This means that my changes will flow through from there. I did as you suggested and my changes were accepted by the OpenViX team.
I'm sorry you feel that this is a merge nightmare. it is not my intention to frustrate your efforts.
Regards,
Ian.
There has been only one commit accepted by OpenViX which relates to "Components/UsageConfig.py" under commit number 06a0a40. This is purely a clean up with no functional changes. This was the first accepted commit and is to test the waters of working with the OpenViX team.
I decided on a clean up only change to find the optimal way of working with the OpenViX team. This wasn't my first submission to OpenViX but it was the first of acceptable quality for them to merge.
When we find a compatible development style I hope to submit the date and time changes that I proposed and discussed here about 8 months ago. Those changes were, at the time, reviewed by others and no problems or objections were reported. Later I would like to move on to proposing other improvements and ideas.
As we are now working on the Beyonwiz firmware that is more closely based on OpenViX you had encouraged me to directly contribute to the OpenViX build. This means that my changes will flow through from there. I did as you suggested and my changes were accepted by the OpenViX team.
I'm sorry you feel that this is a merge nightmare. it is not my intention to frustrate your efforts.
Regards,
Ian.
Re: Pull Request Updates...
Can you two please find a more constructive way of sorting out what is going on so we get the full benefits from your contributions without the extraneous bits?
I'm no programmer but clearly both versions of what's happened cannot be right.
I'm no programmer but clearly both versions of what's happened cannot be right.
Cheers
Gully
_____________
Beyonwiz U4
Logitech Harmony Elite
Google Pixel 6 Pro
Gully
_____________
Beyonwiz U4
Logitech Harmony Elite
Google Pixel 6 Pro
Re: Pull Request Updates...
Actually, they are both right.Gully wrote:I'm no programmer but clearly both versions of what's happened cannot be right.
IanSav is completely right about how I asked him to go about contributing changes and is doing the right thing. And I am completely right about those changes being hard to merge.
I am letting Ian know the nature of the changes that tend to cause issues and some possible ways of avoiding those issues. He is explaining how it happened this time around and that it was not his intent to make it difficult. There's nothing controversial here, move along Gully.
Re: Pull Request Updates...
Well, I stand corrected.peteru wrote:Actually, they are both right.
Well before I leave, maybe just make the posts a bit clearer and more specific on the strategies to avoid those issues as it didn't read like Ian felt helped by your post and reading it did seem more about the problems than the solutions.IanSav is completely right about how I asked him to go about contributing changes and is doing the right thing. And I am completely right about those changes being hard to merge.
I am letting Ian know the nature of the changes that tend to cause issues and some possible ways of avoiding those issues. He is explaining how it happened this time around and that it was not his intent to make it difficult. There's nothing controversial here, move along Gully.
Moving on now.
Cheers
Gully
_____________
Beyonwiz U4
Logitech Harmony Elite
Google Pixel 6 Pro
Gully
_____________
Beyonwiz U4
Logitech Harmony Elite
Google Pixel 6 Pro
Re: Pull Request Updates...
Well, it looks like I just wasted a whole day getting those changes merged (which was a nightmare) only to be greeted with this upstream:
Now the task of undoing all that work begins.Huevos wrote:[UsageConfig] Revert 'PEP8 clean up and tidy up'.
With these changes, automatic merges of commits from other teams to this file would become impossible.
I've explained to Ian.
Re: Pull Request Updates...
Hi PeterU,
I believe the reversion was triggered by your outburst!
Regards,
Ian.
I believe the reversion was triggered by your outburst!
Regards,
Ian.
Re: Pull Request Updates...
I doubt it. I have not been communicating with anyone upstream about this issue.IanSav wrote:Hi PeterU,
I believe the reversion was triggered by your outburst!
Quite simply, the changes you made are a lot of work for minimal benefit.
I think it's more likely that experienced developers came to the conclusion that accepting such a change is just not worth the extra work it's going to create down the track.
Re: Pull Request Updates...
Hi PeterU,
OpenViX team members now participate on this forum.
Regards,
Ian.
OpenViX team members now participate on this forum.
Regards,
Ian.
Re: Pull Request Updates...
Given that is the case, let's save the speculation and accusations and we can ask them to clarify what problems were caused.IanSav wrote:OpenViX team members now participate on this forum.
Regardless of who the other teams were, if it is causing a problem getting the details will highlight what's happening.
Cheers
Gully
_____________
Beyonwiz U4
Logitech Harmony Elite
Google Pixel 6 Pro
Gully
_____________
Beyonwiz U4
Logitech Harmony Elite
Google Pixel 6 Pro
Re: Pull Request Updates...
Ian's contributions are very much appreciated by us.
The reason I reverted the "PEP" pull request was because it is simply a style change.
Positive: It makes the code cleaner and (sometimes) easier to read.
Negative: It makes it impossible to merge and cherry-pick from external sources.
For me the ability to cherry pick is very important. And it needs to work in both directions. i.e. I can cherry-pick from OpenPLi with almost zero merge conflicts. And when I write code I can cherry-pick my new code into my OpenPLi fork and push it back to their image. And also they can easily pick from our image. If we make big changes just for the sake of style that would no longer be possible. So if possible can we stick to only making changes where necessary for run-time benefit.
The reason I reverted the "PEP" pull request was because it is simply a style change.
Positive: It makes the code cleaner and (sometimes) easier to read.
Negative: It makes it impossible to merge and cherry-pick from external sources.
For me the ability to cherry pick is very important. And it needs to work in both directions. i.e. I can cherry-pick from OpenPLi with almost zero merge conflicts. And when I write code I can cherry-pick my new code into my OpenPLi fork and push it back to their image. And also they can easily pick from our image. If we make big changes just for the sake of style that would no longer be possible. So if possible can we stick to only making changes where necessary for run-time benefit.
Re: Pull Request Updates...
Hi,
When Huevos and the other OpenViX developers engaged me in polite and friendly conversation I happily changed and adapted my submission style to suit. The was not one personal attack on me or my skills from *ANY* of the OpenViX team.
Thank you Huevos for your comment and support.
Regards,
Ian.
When Huevos and the other OpenViX developers engaged me in polite and friendly conversation I happily changed and adapted my submission style to suit. The was not one personal attack on me or my skills from *ANY* of the OpenViX team.
Thank you Huevos for your comment and support.
Regards,
Ian.