From: | Khushboo Vashi <khushboo(dot)vashi(at)enterprisedb(dot)com> |
---|---|
To: | Joao Pedro De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io> |
Cc: | Dave Page <dpage(at)pgadmin(dot)org>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>, Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com> |
Subject: | Re: [pgAdmin4][Patch]: Fixed RM #2315 : Sorting by size is broken |
Date: | 2017-04-26 03:48:43 |
Message-ID: | CAFOhELfypnuq+f+SwXhr_4rk29U3oL7rVGOtZHDY_vxXG-0UfQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgadmin-hackers |
Hi Joao & Sarah,
Thanks for reviewing the patch.
On Tue, Apr 25, 2017 at 8:34 PM, Joao Pedro De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:
> Hello Khushboo,
>
> We reviewed the this patch and have some suggestions:
>
> *Python:*
>
> The functionality for adding the "can_prettify" is repeated in multiple
> places. Maybe this could be extracted into a function.
>
> When I have implemented this, my first thought is exactly same as you
suggested but while looking at the code I felt its not a good idea to have
a function. In case of a function, we need to pass the whole result-set as
well as the list of fields which we need to be prettified. So, only for 2
lines, I didn't find any reason to make a function.
> *Javascript:*
>
>
> - The class Backgrid.SizeFormatter doesn't seem to have any tests.
>
>
> Sure, will do.
>
> - The function pg_size_pretty displays bytes and Kilobytes
> differently.
> - Is it possible to add PB as well?
>
> Will check and add PB.
>
> -
> - The function is a little bit hard to read, is it possible to
> refactor using private functions like:
>
> Will make it more readable.
> fromRaw: function (rawData, model) {
> var unitIdx = findDataUnitIndex(rawData);
> if (unitIdx == 0) {
> return rawData + ' ' + this.dataUnits[i];
> }
> return formatOutput(rawData, unitIdx);
> },
>
>
>
>
> - In statistics.js:326 we believe it would make the code more readable
> if we change the variable "c" to "rawColumn" and "col" to "column".
>
>
> I will change the variable name from "c" to "rawColumn" but I think
"col" is appropriate as we already have columns variable and anyone can
confuse while reading the code (for columns and column).
>
> *SQL Files:*
>
>
> - Is there a way to avoid conditionals here?
> - Maybe we can use the same javascript function to prettify all the
> sizes
>
>
> In case of collection node (ex: Databases), I have implemented this
functionality via putting a formatter for a back-grid column. So, it is
applicable for the entire column not for the individual cell. To apply the
javascript function on a single cell for the column (string column), first
we need to identify that cell and then apply the function, I think this is
overhead. Just to avoid conditional sql template I would not prefer this
approach.
>
> Visually we saw a difference between "Databases" statistics and a specific
> database statistics. In "Databases" statistics the "Size" is "7.4 MB" but
> when you are in the specific database the "Size" is "7420 kB".
> Is this the intended behavior?
>
> Only for the Databases (collection node), the client side functionality is
implemented not for individual node , so this behaviour is different. For
the individual node still, we are using pg_size_pretty function
>
>
> Thanks
> Joao & Sarah
>
> On Tue, Apr 25, 2017 at 7:58 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> Ashesh, can you review/commit this please?
>>
>> Thanks.
>>
>> On Tue, Apr 25, 2017 at 10:18 AM, Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi,
>>>
>>> Fixed RM #2315 : Sorting by size is broken.
>>>
>>> Removed the pg_size_pretty function from query for the collection and
>>> introduced the client side function to convert size into human readable
>>> format. So, the sorting issue is fixed as the algorithm will get the actual
>>> value of size instead of formatted value.
>>>
>>>
>>> Thanks,
>>> Khushboo
>>>
>>>
>>>
>>>
>>> --
>>> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>>> To make changes to your subscription:
>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>>
>>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
Thanks,
Khushboo
From | Date | Subject | |
---|---|---|---|
Next Message | Ashesh Vashi | 2017-04-26 04:59:51 | Re: [pgAdmin4][PATCH] To fix the issue with Node rename |
Previous Message | Shirley Wang | 2017-04-25 20:33:05 | Re: [Design update] Style guide for pgAdmin4 |