From: | Dave Page <dpage(at)pgadmin(dot)org> |
---|---|
To: | Murtuza Zabuawala <murtuza(dot)zabuawala(at)enterprisedb(dot)com> |
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 09:00:42 |
Message-ID: | CA+OCxoz94OUBf3hcJHBRJkcVLpqisnOb=+s9QP-Hk1aw0sNCfw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgadmin-hackers |
Thanks, applied.
On Fri, Apr 6, 2018 at 5:53 AM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
> 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(dot)zabuawala(at)enterprisedb(dot)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
>>
>
>
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Dave Page | 2018-04-06 09:06:39 | pgAdmin 4 commit: Update developer docs to note that ES6 should be used |
Previous Message | Dave Page | 2018-04-06 09:00:29 | pgAdmin 4 commit: Add a (configurable) limit to the number of pgAgent j |