Re: [pgadmin-hackers] Possible simple enhancement

On Wed, Sep 16, 2009 at 23:50, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> Le mercredi 16 septembre 2009 à 09:53:26, Magnus Hagander a écrit :
>> On Wed, Sep 16, 2009 at 07:09, Guillaume Lelarge <guillaume@lelarge.info>
> wrote:
>> > Le mardi 15 septembre 2009 à 18:47:24, Guillaume Lelarge a écrit :
>> >> Le mardi 15 septembre 2009 à 09:57:55, Magnus Hagander a écrit :
>> >> > On Tue, Sep 15, 2009 at 07:34, Guillaume Lelarge
>> >> > <guillaume@lelarge.info>
>> >>[...]
>> >> > If we keep that, how about:
>> >> > "The query is longer than the maximum length, and has been truncated.
>> >> > "
>> >>
>> >> Well, we don't really know if it has been truncated. All we know is that
>> >>  the query is at the maximum length.
>> >
>> > New version of the patch:
>> >
>> >  * Previously, only the first 250 characters of the query were displayed.
>> >  * We won't launch the query tool if the selected process is in <IDLE> or
>> >   <IDLE in transaction> state.
>>
>> Quick comment:
>> The logic around getting the max length is wrong. If the query to get
>> it fails, the value of maxlength will be unspecified (the else
>> statement only comes in effect if the version is <8.4). How about just
>> initializing it to 1024 at the start of the function? Also, the
>> "delete set" should be inside the check for NULL value - with a NULL
>> returned you'll attempt to delete NULL.
>>
>
> /me ashamed.
>
> This is fixed.
>
>> >> Should we bother copying at all if it's short?
>> >
>> > Don't understand this one ?!?!
>>
>> I mean if it's cut off, should we actually start a query tool with it,
>> or should we just say "hey, this has been truncated" and *not* start
>> the query tool.
>>
>
> I think we should start it anyway because there is a chance that the query is
> complete.
>
>> >> If we keep that, how about:
>> >> "The query is longer than the maximum length, and has been truncated. "
>> >
>> > Well, we don't really know if it has been truncated. All we know is that
>> > the query is at the maximum length.
>>
>> We don't, but it's pretty likely. So change it to "it may have been
>> truncated"? :-)
>>
>
> Done.
>
> See the new version of this patch.


I think the third sentence in the error message is unnecessary ("you
shuld check this") - that ought to be pretty obvious. But I'm Ok if
it's still there as well, so i'll stop complaining now :-) Go for it!


--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/