Hi Khushboo, That sounds good. Sorry if we weren't clear at first.
Have a good holiday weekend! Sarah & Matt On Fri, Apr 28, 2017 at 4:35 AM, Khushboo Vashi < khushboo.va...@enterprisedb.com> wrote: > Hi Sarah, > > On Thu, Apr 27, 2017 at 7:38 PM, Sarah McAlear <smcal...@pivotal.io> > wrote: > >> Hi Kushboo! >> >> We understand your point, but we believe that relying on 2 independent >> functions to deliver the same formatting can become a problem if the PG >> function changes. Our suggestion is to use a single function in our >> javascript code to do this formatting. >> >> It seems reasonable to me and I am going to use a single javascript > function which will support PB also (as per Dave we should add support till > PB) . > >> If the community believes we can live with this risk, let's move forward. >> >> Thanks! >> Sarah & Joao >> >> On Thu, Apr 27, 2017 at 1:50 AM, Khushboo Vashi < >> khushboo.va...@enterprisedb.com> wrote: >> >>> Hi Joao & Sarah, >>> >>> On Wed, Apr 26, 2017 at 8:46 PM, Joao Pedro De Almeida Pereira < >>> jdealmeidapere...@pivotal.io> wrote: >>> >>>> Hi Khushboo! >>>> >>>> Thanks for your reply! >>>> >>>> >>>>> *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. >>>>> >>>>> >>>> We are not totally sure we understood what you meant on the previous >>>> statement. Are you saying that the conditionals in SQL are used to ensure >>>> that we can apply a javascript function at column level instead of cell >>>> level? >>>> >>>> Correct. >>> >>>> Our concern is that the templates are being made more complex and >>>> inconsistencies are introduced in the code and the UI. >>>> >>> >>> Inconsistencies in the UI can be avoided through making the >>> size_formatter same as pg_size_pretty function which I will implement. >>> I have checked the pg_size_pretty function code and it supports till TB >>> format, so I think we should keep till TB only. >>> >>> In this particular example, we are allowing the backend to respond >>>> sometimes with prettified data and sometimes without it, so at UI level we >>>> will have inconsistencies between screens or more complex Javascript code >>>> to support sometimes prettifying and sometimes not prettify the same >>>> fields. >>>> >>>> We have separate logic for collection and single node in statistics.js >>> and we are using javascript code for prettifying only for collection node. >>> >>> >>>> What we were thinking was to maybe not specify on the SQL level and >>>> have the same format for "Size" everywhere in the UI. >>>> >>>> >>> Here our main concern is inconsistency in "Size" format in the UI that >>> can be avoided as I said earlier. >>> We are using pg_size_pretty function for different fields like Size, >>> Index Size, Table space size, Tuple length, Size of Temporary files in >>> different modules and some of them are cell level and we don't require to >>> put overhead on cell level fields as sorting is not required for individual >>> node statistics. >>> >>> >>> >>>> Thanks >>>> Joao & Sarah >>>> >>>> On Tue, Apr 25, 2017 at 11:48 PM, Khushboo Vashi < >>>> khushboo.va...@enterprisedb.com> wrote: >>>> >>>>> Hi Joao & Sarah, >>>>> >>>>> Thanks for reviewing the patch. >>>>> >>>>> On Tue, Apr 25, 2017 at 8:34 PM, Joao Pedro De Almeida Pereira < >>>>> jdealmeidapere...@pivotal.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 <dp...@pgadmin.org> wrote: >>>>>> >>>>>>> Ashesh, can you review/commit this please? >>>>>>> >>>>>>> Thanks. >>>>>>> >>>>>>> On Tue, Apr 25, 2017 at 10:18 AM, Khushboo Vashi < >>>>>>> khushboo.va...@enterprisedb.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@postgresql.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 >>>>> >>>> >>>> >>> Thanks, >>> Khushboo >>> >> >> > Thanks, > Khushboo >