Need feedback on first four UTF-7 patches
Alexandre, I submitted four UTF-7 patches on October 19 and am still waiting for feedback. I was hoping that these patches would not be controversial because they only add encoding tests. I have six additional patches waiting in https://github.com/alexhenrie/wine/commits/master - four patches for adding decoding tests and two patches to make all the tests pass - but I will not submit them until I get feedback about the first four that I have already submitted. -Alex
Hi Alex, If you look at http://source.winehq.org/patches/ , you'll see that there're some patches which are a month old, but still having "New" status. So please be patient, your ones are not that old yet :) Regards, Ruslan On Tue, Oct 28, 2014 at 11:13 PM, Alex Henrie <alexhenrie24(a)gmail.com> wrote:
Alexandre,
I submitted four UTF-7 patches on October 19 and am still waiting for feedback. I was hoping that these patches would not be controversial because they only add encoding tests. I have six additional patches waiting in https://github.com/alexhenrie/wine/commits/master - four patches for adding decoding tests and two patches to make all the tests pass - but I will not submit them until I get feedback about the first four that I have already submitted.
-Alex
Well, indeed, but is this really a good target to strive for and bring out as an example? The 4 patches add only a number of tests that verify difference between wine and windows UTF-7 encoding, missing stuff is marked as wine_todo, testbot has marked them as passing (meaning todo_wine marked tests are expected to fail on current wine and do so and same tests are supposed to pass on windows and do so), no actual implementation that would change the behavior of wine itself. So unless there is something wrong with the coding style (which hasn't been commented on after the last patches), shouldn't such patches (only tests, verified as valid by testbot) get included a lot faster than 10-30 days?? Regards, Indrek Altpere -----Original Message----- From: wine-devel [mailto:wine-devel-bounces(a)winehq.org] On Behalf Of Ruslan Kabatsayev Sent: Wednesday, October 29, 2014 9:48 AM To: Alex Henrie; Alex Henrie Cc: Alexandre Julliard; Wine Devel; Alexandre Julliard; Wine Devel Subject: Re: Need feedback on first four UTF-7 patches Hi Alex, If you look at http://source.winehq.org/patches/ , you'll see that there're some patches which are a month old, but still having "New" status. So please be patient, your ones are not that old yet :) Regards, Ruslan On Tue, Oct 28, 2014 at 11:13 PM, Alex Henrie <alexhenrie24(a)gmail.com> wrote:
Alexandre,
I submitted four UTF-7 patches on October 19 and am still waiting for feedback. I was hoping that these patches would not be controversial because they only add encoding tests. I have six additional patches waiting in https://github.com/alexhenrie/wine/commits/master - four patches for adding decoding tests and two patches to make all the tests pass - but I will not submit them until I get feedback about the first four that I have already submitted.
-Alex
On 10/29/2014 09:47 AM, Indrek Altpere wrote: > Well, indeed, but is this really a good target to strive for and bring out > as an example? > The 4 patches add only a number of tests that verify difference between wine > and windows UTF-7 encoding, missing stuff is marked as wine_todo, testbot > has marked them as passing (meaning todo_wine marked tests are expected to > fail on current wine and do so and same tests are supposed to pass on > windows and do so), no actual implementation that would change the behavior > of wine itself. > So unless there is something wrong with the coding style (which hasn't been > commented on after the last patches), shouldn't such patches (only tests, > verified as valid by testbot) get included a lot faster than 10-30 days?? > > Regards, > Indrek Altpere Keep in mind that developers time is very scarce because they (amongst many other tasks) : - read bugzilla reports - install the app, trigger the bug - scratch some patch to aid debugging - test expected behavior on windows machines (real or virtual) - refine their patch - check for non-regression with the testbot - sometimes, ask for comments on wine-devel but they also - read patches from the patch queue when they are confident in this wine area (check coding style, variable names, logic, error handling...) - test and comment them - read technical specifications - write lengthy e-mails when things are complicated AJ is doing all of that *plus* he manages his team and manages *all the* patches queue They are doing that every day for many years and even if it's perfectible, don't waste their precious time and stop whining Please be patient, if nothing happens, try doing a more convincing argument (add tests, reference bug id you solve...) and finally, resend your patch
On 29 October 2014 09:47, Indrek Altpere <efbiaiinzinz(a)hotmail.com> wrote:
So unless there is something wrong with the coding style (which hasn't been commented on after the last patches), shouldn't such patches (only tests, verified as valid by testbot) get included a lot faster than 10-30 days??
In general, yes, if your patches are still "New" after about a week, they're probably going to stay that way. That usually only happens when not reviewing the patches is likely to end up being more of a benefit to the project than reviewing them.
My previous response was mainly to Ruslan's reply, that 30 days waiting time could/should be considered the norm in some cases. To which I pointed out that the 4 patches Alex asked about are seemingly simple patches, that only add tests and those patches have already been verified to run correctly by testbot. To Goujons's reply: "don't waste their precious time and stop whining" As per Alex's comment from https://www.winehq.org/pipermail/wine-devel/2014-October/105419.html "slackner, puk, and Andre_H gave me a ton of help over IRC over the past couple of days" Time has already been spent (wasted?) by other active Wine developers who are also constantly contributing to Wine. Perhaps there should be some more clear way to let the developers know that their patches are still not good enough (even to be reviewed and commented on) instead of checking the calendar to guess the status? Regards, Indrek Altpere -----Original Message----- From: Henri Verbeet [mailto:hverbeet(a)gmail.com] Sent: Wednesday, October 29, 2014 1:58 PM To: Indrek Altpere Cc: Ruslan Kabatsayev; Alex Henrie; Alexandre Julliard; Wine Devel Subject: Re: Need feedback on first four UTF-7 patches On 29 October 2014 09:47, Indrek Altpere <efbiaiinzinz(a)hotmail.com> wrote:
So unless there is something wrong with the coding style (which hasn't been commented on after the last patches), shouldn't such patches (only tests, verified as valid by testbot) get included a lot faster than 10-30 days??
In general, yes, if your patches are still "New" after about a week, they're probably going to stay that way. That usually only happens when not reviewing the patches is likely to end up being more of a benefit to the project than reviewing them.
On 29 October 2014 13:21, Indrek Altpere <efbiaiinzinz(a)hotmail.com> wrote:
My previous response was mainly to Ruslan's reply, that 30 days waiting time could/should be considered the norm in some cases.
Yeah, it's not.
Perhaps there should be some more clear way to let the developers know that their patches are still not good enough (even to be reviewed and commented on) instead of checking the calendar to guess the status?
The patch status page seems pretty clear to me. Put a different way, suppose you e.g. had additional information about why a patch is still "New", what would you do with that information?
Well, looking at the list, there are patches that are marked as rejected: those are pretty clear that there is some issue with the patch/patchset. But seeing a patch row marked as New and for which testbot runs have completed successfully gives no definitive feedback about the actual status. There is no way to differentiate if a patch has been forgotten accidentally, or ignored on purpose (for the benefit of the project) or postponed for later review (in a week or two). For me personally, in this case, I wouldn't do much with the information since I'm not the author of those patches nor did I help review them. But it would conserve time for both the developers and reviewers if there is a clearer marker of status, instead of guessing it via calendar and/or polling the wine-devel list. Regards, Indrek Altpere -----Original Message----- From: Henri Verbeet [mailto:hverbeet(a)gmail.com] Sent: Wednesday, October 29, 2014 2:42 PM To: Indrek Altpere Cc: Ruslan Kabatsayev; Alex Henrie; Alexandre Julliard; Wine Devel Subject: Re: Need feedback on first four UTF-7 patches On 29 October 2014 13:21, Indrek Altpere <efbiaiinzinz(a)hotmail.com> wrote:
My previous response was mainly to Ruslan's reply, that 30 days waiting time could/should be considered the norm in some cases.
Yeah, it's not.
Perhaps there should be some more clear way to let the developers know that their patches are still not good enough (even to be reviewed and commented on) instead of checking the calendar to guess the status?
The patch status page seems pretty clear to me. Put a different way, suppose you e.g. had additional information about why a patch is still "New", what would you do with that information?
On 29 October 2014 14:05, Indrek Altpere <efbiaiinzinz(a)hotmail.com> wrote:
But seeing a patch row marked as New and for which testbot runs have completed successfully gives no definitive feedback about the actual status. There is no way to differentiate if a patch has been forgotten accidentally, or ignored on purpose (for the benefit of the project) or postponed for later review (in a week or two).
I get that, but what I'm saying is that in either case you're not going to make the patch go in quicker by e.g. sending another three versions of the same patch to the patches list. The first case is pretty rare, and I don't think having a separate "Forgotten" state would really help there. The second and third case are perhaps more interesting, but not necessarily distinct. It's not so much that when reviewing patches you arbitrarily decide to not look at a patch for two weeks, but rather that if it's e.g. a slow day you may review a couple of patches that you normally wouldn't have bothered with.
On 10/29/2014 01:21 PM, Indrek Altpere wrote:
My previous response was mainly to Ruslan's reply, that 30 days waiting time could/should be considered the norm in some cases. To which I pointed out that the 4 patches Alex asked about are seemingly simple patches, that only add tests and those patches have already been verified to run correctly by testbot.
To Goujons's reply: "don't waste their precious time and stop whining" As per Alex's comment from https://www.winehq.org/pipermail/wine-devel/2014-October/105419.html
"slackner, puk, and Andre_H gave me a ton of help over IRC over the past couple of days"
Time has already been spent (wasted?) by other active Wine developers who are also constantly contributing to Wine.
Perhaps there should be some more clear way to let the developers know that their patches are still not good enough (even to be reviewed and commented on) instead of checking the calendar to guess the status? Oh, we have provided a ton of feedback. And the patches did improve from a test coverage point of view and that isn't anymore an issue.
But the code still could look a lot better and some of the previous feedback wasn't addressed. There is an impedance mismatch between the code looking good for Alex and looking good for Wine. bye michael
-----Original Message----- From: Henri Verbeet [mailto:hverbeet(a)gmail.com] Sent: Wednesday, October 29, 2014 1:58 PM To: Indrek Altpere Cc: Ruslan Kabatsayev; Alex Henrie; Alexandre Julliard; Wine Devel Subject: Re: Need feedback on first four UTF-7 patches
On 29 October 2014 09:47, Indrek Altpere <efbiaiinzinz(a)hotmail.com> wrote:
So unless there is something wrong with the coding style (which hasn't been commented on after the last patches), shouldn't such patches (only tests, verified as valid by testbot) get included a lot faster than 10-30 days??
In general, yes, if your patches are still "New" after about a week, they're probably going to stay that way. That usually only happens when not reviewing the patches is likely to end up being more of a benefit to the project than reviewing them.
2014-10-29 6:45 GMT-06:00 Michael Stefaniuc <mstefani(a)redhat.com>:
But the code still could look a lot better and some of the previous feedback wasn't addressed. There is an impedance mismatch between the code looking good for Alex and looking good for Wine.
As far as I know, I only rejected one of your suggestions for these 4 patches: Merging the simple_test and complex_test structs. The 3 simple tests are actually used as inputs for 8 tests each, for a total of 24 tests. Reformatting the 3 simple tests as complex tests would force me to manually input the 24 combinations into the complex_tests array. Is that really what Alexandre wants? -Alex
On 29.10.2014 20:50, Alex Henrie wrote:
2014-10-29 6:45 GMT-06:00 Michael Stefaniuc <mstefani(a)redhat.com>:
But the code still could look a lot better and some of the previous feedback wasn't addressed. There is an impedance mismatch between the code looking good for Alex and looking good for Wine.
As far as I know, I only rejected one of your suggestions for these 4 patches: Merging the simple_test and complex_test structs. The 3 simple tests are actually used as inputs for 8 tests each, for a total of 24 tests. Reformatting the 3 simple tests as complex tests would force me to manually input the 24 combinations into the complex_tests array. Is that really what Alexandre wants?
-Alex
It is not really true that this was the only thing we criticized. We also gave you a lot more suggestions for improvements, but from the beginning I had the impression that you want to keep changes as minimal as possible. Thats not a good attitude when trying to upstream code, and will probably be especially a problem for the implementation itself. You always have to consider multiple methods how a specific problem can be solved. For the tests: The main issue (bad test coverage) is fixed at my opinion, but there are still lot of small things which could be improved to make the code more readable. Most of it was already told on IRC, but I will go over patch 1 one more time and tell you again.
2014-10-29 14:37 GMT-06:00 Sebastian Lackner <sebastian(a)fds-team.de>:
For the tests: The main issue (bad test coverage) is fixed at my opinion, but there are still lot of small things which could be improved to make the code more readable. Most of it was already told on IRC, but I will go over patch 1 one more time and tell you again.
Again, thanks for the feedback. For the record, I am holding off on the following suggestions until I receive feedback from Alexandre: - Merging the complex and simple structs (this would decrease test coverage) - Splitting up the ok() statements into pieces not joined by && (this seems like overkill) - Rebasing the non-struct-based tests to be ahead of the struct-based tests in the patch series (this would make it difficult to write a simple check for the presence or absence of UTF-7 support) You can see that despite my frustration with this process, I have incorporated the rest of your suggestions at https://github.com/alexhenrie/wine/commits/master -Alex
Alex Henrie <alexhenrie24(a)gmail.com> writes:
2014-10-29 14:37 GMT-06:00 Sebastian Lackner <sebastian(a)fds-team.de>:
For the tests: The main issue (bad test coverage) is fixed at my opinion, but there are still lot of small things which could be improved to make the code more readable. Most of it was already told on IRC, but I will go over patch 1 one more time and tell you again.
Again, thanks for the feedback.
For the record, I am holding off on the following suggestions until I receive feedback from Alexandre:
If you want more feedback, you should first address all the issues that you have already received feedback about. -- Alexandre Julliard julliard(a)winehq.org
participants (8)
-
Alex Henrie -
Alexandre Julliard -
GOUJON Alexandre -
Henri Verbeet -
Indrek Altpere -
Michael Stefaniuc -
Ruslan Kabatsayev -
Sebastian Lackner