Re: [pgAdmin4][RM#3235] Code refactoring in Query tool
Thanks, applied.
On Wed, Apr 4, 2018 at 9:43 AM, Murtuza Zabuawala <
murtuza.zabuaw...@enterprisedb.com> wrote:
> Hi,
>
> Thank you Victoria & Joao for reviewing.
> PFA updated patch.
>
> On Wed, Apr 4, 2018 at 12:26 AM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.io> 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 <
>> murtuza.zabuaw...@enterprisedb.com> 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 wrote:
>>>
HI
On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala <
murtuza.zabuaw...@enterprisedb.com> 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
Re: [pgAdmin4][RM#3235] Code refactoring in Query tool
Hi,
Thank you Victoria & Joao for reviewing.
PFA updated patch.
On Wed, Apr 4, 2018 at 12:26 AM, Joao De Almeida Pereira <
jdealmeidapere...@pivotal.io> 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 enterprisedb.com> 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 wrote:
>>
>>> HI
>>>
>>> On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala >> enterprisedb.com> 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
>>>
>>
>>
RM_3235_v2.diff
Description: Binary data
Re: [pgAdmin4][RM#3235] Code refactoring in Query tool
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
)
- 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.
- 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:
httpResponseRequiresNewTransaction
- 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.
- 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.
Thanks
Victoria & Joao
On Tue, Apr 3, 2018 at 11:38 AM Murtuza Zabuawala <
murtuza.zabuaw...@enterprisedb.com> 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 wrote:
>
>> HI
>>
>> On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala <
>> murtuza.zabuaw...@enterprisedb.com> 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
>>
>
>
Re: [pgAdmin4][RM#3235] Code refactoring in Query tool
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 wrote: > HI > > On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala enterprisedb.com> 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 > RM_3235_v1.diff Description: Binary data
Re: [pgAdmin4][RM#3235] Code refactoring in Query tool
HI On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala < murtuza.zabuaw...@enterprisedb.com> 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
[pgAdmin4][RM#3235] Code refactoring in Query tool
Hi, PFA patch to extract the common code from query tool to handle ajax errors & connection handling, Also added unit tests around extracted code. -- Regards, Murtuza Zabuawala EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company RM_3235.diff Description: Binary data
