Re: [pgAdmin4][RM#3072] Make pgagent job history rows configurable

From: Murtuza Zabuawala <murtuza(dot)zabuawala(at)enterprisedb(dot)com>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: Joao De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: [pgAdmin4][RM#3072] Make pgagent job history rows configurable
Date: 2018-04-06 04:53:07
Message-ID: CAKKotZQN5t40qfq7LJC0_oNMh3NYS9z7HdkFy1Xztj=jYbMJpg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Hi,

Please find updated patch which fixes PEP8 issue, added help string for
preference dialog option & updated screenshot.

--
Regards,
Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Apr 5, 2018 at 7:56 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Sorry Murtuza - I'm mixing patches up here. It's the sort/filter one I
> need the image for please.
>
> Which means this patch needs re-creating as well please.
>
> On Thu, Apr 5, 2018 at 3:03 PM, Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> Please find attached PNG.
>>
>> On Thu, Apr 5, 2018 at 7:25 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Murtuza, please send me the screenshot as a PNG. I've been tweaking the
>>> code and will recreate the patch with my changes.
>>>
>>> On Thu, Apr 5, 2018 at 2:53 PM, Joao De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>
>>>> Hi Murtuza,
>>>> Generally the patch looks good passes all CI but the linter fails:
>>>>
>>>> /tmp/build/4a5630c2/pivotal-rm-3072/web /tmp/build/4a5630c2
>>>> <https://gpdb-dev.bosh.pivotalci.info/teams/main/pipelines/pgadmin-feature-branches/jobs/pivotal-rm-3072-python-linter/builds/1#L5ab9877e:9>
>>>> ./pgadmin/browser/register_browser_preferences.py:11: [E302] expected 2 blank lines, found 1
>>>> <https://gpdb-dev.bosh.pivotalci.info/teams/main/pipelines/pgadmin-feature-branches/jobs/pivotal-rm-3072-python-linter/builds/1#L5ab9877e:10>
>>>> 1 E302 expected 2 blank lines, found 1
>>>> <https://gpdb-dev.bosh.pivotalci.info/teams/main/pipelines/pgadmin-feature-branches/jobs/pivotal-rm-3072-python-linter/builds/1#L5ab9877e:11>
>>>> 1
>>>>
>>>>
>>>> Did not test the functionality itself ....
>>>>
>>> ​(at)Joao,
>> I have included the test for Stats call which will cover the newly added
>> functionality.​
>>
>>
>>> Thanks
>>>> Joao
>>>>
>>>> On Thu, Apr 5, 2018 at 8:09 AM Murtuza Zabuawala <
>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Dave,
>>>>>
>>>>> Please find update patch.
>>>>>
>>>>> --
>>>>> Regards,
>>>>> Murtuza Zabuawala
>>>>> EnterpriseDB: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>>
>>>>> On Thu, Apr 5, 2018 at 4:52 PM, Murtuza Zabuawala <
>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> On Thu, Apr 5, 2018 at 4:43 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> On Thu, Apr 5, 2018 at 11:10 AM, Murtuza Zabuawala <
>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> PFA patch which allow user to configure how many rows they wish to
>>>>>>>> display for any pgagent jobs on statistics panel.
>>>>>>>>
>>>>>>>
>>>>>>> I think this is essentially good, however, I'm really not happy with
>>>>>>> the preference name and category. In general, I'd suggest that before
>>>>>>> creating patches in the future we should confirm naming etc on the mailing
>>>>>>> list, as I often end up changing wording and then requiring new screenshots
>>>>>>> etc.
>>>>>>>
>>>>>> ​Ok​
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> In this case, I really don't like that we've added another category,
>>>>>>> and quite a specific one at that. I would suggest we move it to Browser ->
>>>>>>> Properties and name it "Maximum job history rows" with a description of
>>>>>>> "The maximum number of history rows to show on the Statistics tab for
>>>>>>> pgAgent jobs."
>>>>>>>
>>>>>> I'll change it and resend the patch.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thoughts?
>>>>>>>
>>>>>>> --
>>>>>>> Dave Page
>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>> Twitter: @pgsnake
>>>>>>>
>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Attachment Content-Type Size
RM_3072_v2.diff application/octet-stream 231.2 KB

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Murtuza Zabuawala 2018-04-06 05:44:41 Re: [pgAdmin4][RM#3055] Allow user to sort the data in View data mode
Previous Message Murtuza Zabuawala 2018-04-06 03:43:06 Re: [pgAdmin4][RM#3055] Allow user to sort the data in View data mode