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.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 <dp...@pgadmin.org> wrote: >> >>> HI >>> >>> On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala <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
