From: <fb...@li...> - 2013-07-11 03:59:13
|
Hi Cristian,with your changes in fics.tcl the line 1299:if { $elt == "Click or type \[next\] to see next page." }will never be true (for example typing info into fics console and clicking on "Click or type [next] to see next page" no longer works).Bye,Fulvio ----Messaggio originale---- Da: cri...@gm... Data: 10/07/2013 22.17 A: "Fulvio"<fb...@li...> Cc: "scid-users"<sci...@li...> Ogg: minor patch for fics window Hi Fulvio, Please review and pull the following patches from fix_proper_list branch. Cristian S. |
From: <fb...@li...> - 2013-07-11 22:07:37
|
The following code: set elt "one two three" puts [lindex $elt 2] set elt [split $elt " "]puts [lindex $elt 2] will output:threetwo that's why i believe that the patch is risky and require accurate testing (and probably it's not the right approach). Bye,Fulvio ----Messaggio originale---- Da: cri...@gm... Data: 11/07/2013 18.51 A: "fb...@li..."<fb...@li...> Cc: "scid-users"<sci...@li...> Ogg: Re: minor patch for fics window Hi Fulvio, Please try branch fix_proper_list2 for an updated patch. Cristian S. |
From: Cristian S. <cri...@gm...> - 2013-07-12 08:05:25
|
You could argue the reverse if the patch was already there and then removed for not adding visible value. But not unless there was a real case where the removal would regress (e.g. because the parsed text is not regular). Does the addition regress? The fact that 'split' adds an empty element in your example might suggest the patch is going to fail in some way, but the original code did not bother with such cases nor did it consider that the list should be properly formatted for robustness. If the format of the parsed text is that troublesome, then the original code needs an overhaul. How does the parsed text look like? Cristian S. |
From: Fulvio <fb...@li...> - 2013-07-12 09:41:28
|
Cristian Stoica wrote: > You could argue the reverse if the patch was already there and then > removed Exactly, but the current code is in use by more than 4 years and there are no bug reports. I'm not saying that adding "split" is wrong; the point is that it makes the code more robust, but it also changes the parsing logic. Sure, we could simply commit the code and let users find if it add any bugs, but i think it's not the right way. It's better to check if the fics protocol provide any guarantee against multiple spaces, think about possible corner cases, try to find better solutions for parsing, etc... (it's a lot of tedious work -> i'm not doing it :-) -> but need to be done before committing the change). Bye, Fulvio |
From: Cristian S. <cri...@gm...> - 2013-07-12 12:41:59
|
>...but the current code is in use by more than 4 years and there are no bug reports. No bug reports doesn't mean there are no bugs. For example, I have three of them in my queue (two for fics) that I did not report. Sure, we could simply commit the code and let users find if it add any > bugs, but i think it's not the right way. > I'm against taking patches without review and testing. I fall into this trap myself times and times again. I needed to know if the example was to make a point (it does) or if you extracted it from a real regression (I guess not but I don't know fics protocol enough to make a call). It's better to check if the fics protocol provide any guarantee against > multiple spaces, think about possible corner cases, try to find better > solutions for parsing, etc... (it's a lot of tedious work -> i'm not doing > it :-) Reducing entropy is always going to take energy. I am willing to put some in. And I guess we all like some areas better than others. Speaking of it, what about four space tab-stops for new and updated code (of course not for the sake of it)? Cristian S. |
From: Fulvio <fb...@li...> - 2013-07-12 14:21:46
|
Cristian Stoica wrote: > Sure, we could simply commit the code and let users find if it add any > bugs, but i think it's not the right way. > I'm against taking patches without review and testing. I fall into > this trap myself times and times again. I needed to know if the > example was to make a point (it does) or if you extracted it from a > real regression (I guess not but I don't know fics protocol enough to > make a call). Yes, i didn't try the patch, i just wanted to point out that need more testing > > Speaking of it, what about four space tab-stops for new and updated > code (of course not for the sake of it)? In the eternal conflict between tabs vs spaces i'm a tabs supporter too. However in scid we try to maintain file consistency: when patching a file that used spaces you should use spaces, when patching a file that used tabs you should use tabs. If you code a new feature that deserve his own file you are free to choose (even if i hope that everyone see the light and stop using spaces :-) Bye, Fulvio |
From: Cristian S. <cri...@gm...> - 2013-07-12 15:28:29
|
> Speaking of it, what about four space tab-stops for new and updated code >> (of course not for the sake of it)? >> > In the eternal conflict between tabs vs spaces i'm a tabs supporter too. > However in scid we try to maintain file consistency: when patching a file > that used spaces you should use spaces, when patching a file that used tabs > you should use tabs. > If you code a new feature that deserve his own file you are free to choose > (even if i hope that everyone see the light and stop using spaces :-) > I don't care about tabs vs spaces but what about two-space tabs? Do you want to keep them as they are even if you prefer otherwise? Cristian S. |
From: Cristian S. <cri...@gm...> - 2013-07-11 06:46:17
|
Thanks, I should have payed more attention to the context (and working late is overrated). I'll send you an update this weekend. Cristian S. |
From: Cristian S. <cri...@gm...> - 2013-07-11 16:51:41
|
Hi Fulvio, Please try branch fix_proper_list2 for an updated patch. Cristian S. |