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 <dp...@pgadmin.org> 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
