public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dave Page <[email protected]>
To: Murtuza Zabuawala <[email protected]>
Cc: Joao De Almeida Pereira <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin4][RM#3235] Code refactoring in Query tool
Date: Wed, 4 Apr 2018 11:21:04 +0100
Message-ID: <CA+OCxoywrYPUQga7FHywv0g0K=hqVtP8h0vtChyn4pjV07o-ag@mail.gmail.com> (raw)
In-Reply-To: <CAKKotZS5pAJ70iNJkvUTLr5jZ7nJNArwkiQyfgaz3y9csdarWg@mail.gmail.com>
References: <CAKKotZRidfdPkSe=2wiaDsz_F3sAXdWZs_1SQLFks1YeP24UTQ@mail.gmail.com>
<CA+OCxoygT-+xezSAboURa9E9myQCX2GbO4DCFg7yY+umHhEa5g@mail.gmail.com>
<CAKKotZRwxcozrSic25xSCtK_z7bdnksrF2_4GSeWi=6zDUSw_g@mail.gmail.com>
<CAE+jjang5oqj8-c_VhryAET8aaj6USYtRuLmY8gKctC8wz475w@mail.gmail.com>
<CAKKotZS5pAJ70iNJkvUTLr5jZ7nJNArwkiQyfgaz3y9csdarWg@mail.gmail.com>
Thanks, applied.
On Wed, Apr 4, 2018 at 9:43 AM, Murtuza Zabuawala <
[email protected]> wrote:
> Hi,
>
> Thank you Victoria & Joao for reviewing.
> PFA updated patch.
>
> On Wed, Apr 4, 2018 at 12:26 AM, Joao De Almeida Pereira <
> [email protected]> wrote:
>
>> Hi Murtuza
>>
>> It is really good to see other patches that are just refactoring code.
>>
>> We have some suggestions:
>> - We are trying to use === instead of == because === ensure that the type
>> is also checked (https://developer.mozilla.org
>> /en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness)
>>
> Done
>
>
> - Now that we are refactoring some code, maybe we should keep some
>> consistency on the way we name functions and variables.
>>
> We should use camelCase for names instead of snake_case. In general, if
>> you see a function or variable name that doesn't fit the desired syntax or
>> if a block of code seems confusing, it is better to refactor it.
>>
> Done, I have also changed other variables.
> BTW I'm using camelCase convention from last few patches :)
>
> - By the name of the function is_new_transaction_required, it describes
>> what the function represents but doesn't seem to explain the full scope of
>> the function. What do you think about the name: httpResponseRequiresNewT
>> ransaction
>>
> Done
>
>
> - The extraction doesn't look like it matches the code removed
>>
>
>> - if (pgAdmin.Browser.UserManagement.is_pga_login_required(e))
>> {
>> - self.save_state('_explain_timing', []);
>> - return pgAdmin.Browser.UserManagement.pga_login();
>> - }
>> -
>> - if(transaction.is_new_transaction_required(e)) {
>> - self.save_state('_explain_timing', []);
>> - return self.init_transaction();
>> - }
>> -
>> - alertify.alert(gettext('Explain options error'),
>> - gettext('Error occurred while setting timing option in
>> explain.')
>> + let msg = httpErrorHandler.handleQueryToolAjaxError(
>> + pgAdmin, self, e, '_explain_timing', null, true
>> );
>> + alertify.alert(gettext('Explain options error'), msg);
>> In this case we are only saving state if the following conditions are
>> true:
>> isNotConnectedToThePythonServer and httpResponseJSONIsPresent and
>> connectionLostToPostgresDatabase and shouldSaveState
>> That is not the case on the removed code.
>>
> I think the *null* value got your attention b
> ut actually I have a check in *handleQueryToolAjaxError *which will make
> it array [] with respect to arguments for the state to save, Anyways I have
> also changed it to pass [] instead of null for better clarity.
> We have all those checks in our function so it check for those condition
> and save the state only if those returns True.
>
> - The functions extracted when are called do not use all the parameters.
>> This tells us that the function groups too much functionality that is not
>> used in same cases. Maybe we should split this function into smaller
>> functions that do each part.
>>
> We already had split up the function in two part.
>
>
>>
>>
>> Thanks
>> Victoria & Joao
>>
>> On Tue, Apr 3, 2018 at 11:38 AM Murtuza Zabuawala <
>> [email protected]> wrote:
>>
>>> Hi Dave,
>>>
>>> PFA updated patch, I've renamed it to query_tool_http_error_handler.js
>>> & query_tool_http_error_handler_spec.js respectively.
>>>
>>> --
>>> Regards,
>>> Murtuza Zabuawala
>>> EnterpriseDB: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>>
>>> On Tue, Apr 3, 2018 at 7:43 PM, Dave Page <[email protected]> wrote:
>>>
>>>> HI
>>>>
>>>> On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala <
>>>> [email protected]> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> PFA patch to extract the common code from query tool to handle ajax
>>>>> errors & connection handling, Also added unit tests around extracted code.
>>>>>
>>>>
>>>> Looks good to me, except, I wonder if we should rename
>>>> is_new_transaction_required.js/is_new_transaction_required_spec.js to
>>>> something a little more generic; maybe conn_tx_handler_funcs.js? Not sure I
>>>> like that though.
>>>>
>>>>
>>>> --
>>>> 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
view thread (6+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [pgAdmin4][RM#3235] Code refactoring in Query tool
In-Reply-To: <CA+OCxoywrYPUQga7FHywv0g0K=hqVtP8h0vtChyn4pjV07o-ag@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox