Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
От | Harshal Dhumal |
---|---|
Тема | Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch] |
Дата | |
Msg-id | CAFiP3vzxqm6Utwf4uWXZ=B267W4apToLG8WYVcjG6Q9xSNqgGQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch] (Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io>) |
Ответы |
Re: [pgadmin-hackers] Fix for RM2421 [pgAdmin4][patch]
(Harshal Dhumal <harshal.dhumal@enterprisedb.com>)
|
Список | pgadmin-hackers |
Hi,
On Thu, May 18, 2017 at 7:57 PM, Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hello Harshal,We review the patch and have some questions:1) Is there any particular reason to initialize variables and functions in the same place? We believe that it would be more readable there were no chaining of variable creation, specially if those variables are functions. Check line:
That function is only going to be used in checkNumeric function (in case of Number control) and checkInt function (in case of Integer control) so declared them locally.
Anyway I'm going to refactor both the controls as Number and Integer shares some common properties.
+++ b/web/pgadmin/static/js/
backform.pgadmin.js @@ -1528,7 +1528,18 @@ max_value = field.max, isValid = true, intPattern = new RegExp("^-?[0-9]*$"), - isMatched = intPattern.test(value); + isMatched = intPattern.test(value), + trigger_invalid_event = function(msg) { 2) The functions added in both places look very similar, can they be merged and extracted? We are talking about the trigger_invalid_eventfunction.
Yes they can be merged. As of now both NumericControl and IntegerControl are derived from InputControl. Ideally
only NumericControl should be derived from InputControl and IntegerControl should be derive from NumericControl.
3) The following change is very similar to the trigger_invalid_event, was there a reason not to use it?
Below code triggers "model valid" event; opposite to "model invalid" event (trigger_invalid_event)
+++ b/web/pgadmin/static/js/
backform.pgadmin.js @@ -1573,25 +1584,23 @@ this.model.errorModel.unset( name); this.model.set(name, value); this.listenTo(this.model, "change:" + name, this.render); - if (this.model.collection || this.model.handler) { - (this.model.collection || this.model.handler).trigger( - 'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler) - ); + // Check if other fields of same model are valid before + // triggering 'session:valid' event + if(_.size(this.model. errorModel.attributes) == 0) { + if (this.model.collection || this.model.handler) { + (this.model.collection || this.model.handler).trigger( + 'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler) + ); + } else { + (this.model).trigger( + 'pgadmin-session:valid', this.model.sessChanged(), this.model + ); + } 4) We also noticed that the following change sets look very similiar. Is there any reason to have this code duplicated? If not this could be a good time to refactor it.
As said earlier in response of point 2 code duplication is because the way controls are derived.
+++ b/web/pgadmin/static/js/
backform.pgadmin.js @@ -1528,7 +1528,18 @@ @@ -1573,25 +1584,23 @@ @@ -1631,7 +1640,18 @@ @@ -1676,25 +1696,23 @@ ThanksJoao & ShrutiOn Thu, May 18, 2017 at 6:01 AM, Harshal Dhumal <harshal.dhumal@enterprisedb.com> wrote: --Hi,Please find attached patch for RM2421Issue fixed: 1. Integer/numeric Validation is not working properly.2. Wrong CPU rate unit--Harshal DhumalSr. Software Engineer
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
В списке pgadmin-hackers по дате отправления:
Предыдущее
От: Akshay JoshiДата:
Сообщение: Re: [pgadmin-hackers] Declarative partitioning in pgAdmin4
Следующее
От: Dave PageДата:
Сообщение: [pgadmin-hackers] pgAdmin 4 commit: Tag REL-1_5 has been created.