Hi Joao On Tue, Apr 24, 2018 at 10:04 PM, Joao De Almeida Pereira <jdealmeidapereira @pivotal.io> wrote:
> Hi Akshay, > > After looking through the patch we found some one letter variable names > and this is a regression on what we have been trying to accomplish in the > last year. > > An objective that we have for pgAdmin source code is to increase the > testability of it and make it more readable. If we keep on adding one > letter variables and if we continue adding code to already convoluted > source files it is going to be very hard to achieve this objective. > At my level I have tried not to give one letter variable names. Are you talking about the variable "m" in server.js file which represents the Model? If yes then I have followed the code written for whole schema and I thought we have to maintain the consistency, so use that as it is. Apart from that I haven't seen any other one letter variable, please correct me so that I'll rename it. > > Our recommendations for this change are: > - Name the variables with comprehensive names > Can you please suggest from the patch. > - Extract functions where we can and try to wrap some tests around them > (ex: the javascript disabled functions) > I have tried to do that too, if you can see the "server/__init__.py" file I have created "*get_response_for_password" function to remove redundant code. Based on the condition it will return the json response.* > - We really need to find a better pattern than templated Javascript to > pass information from the backend to the frontend > - When changing a piece of code, if we see code that is confusing or that > is hard to read, we should refactor instead of adding to the pattern. > Please elaborate more with respect to my patch, which part of code should required modification? > > Thanks > Victoria & Joao > > > On Tue, Apr 24, 2018 at 10:13 AM Akshay Joshi < > akshay.jo...@enterprisedb.com> wrote: > >> Hi Hackers >> >> As per suggestion by Dave, I have moved "Advanced" tab at the last for >> Server dialog. Attached is the modified patch. >> >> On Mon, Apr 23, 2018 at 7:32 PM, Anthony Emengo <aeme...@pivotal.io> >> wrote: >> >>> For what it is worth, I manually verified that the feature worked, as >>> well as looked through the code. >>> >>> I'd like to see end-to-end testing for regression sake, but it's hard to >>> so at this moment. >>> >>> - Anthony and Joao. >>> >>> On Mon, Apr 23, 2018 at 5:09 AM, Akshay Joshi < >>> akshay.jo...@enterprisedb.com> wrote: >>> >>>> >>>> >>>> On Mon, Apr 23, 2018 at 1:30 PM, Dave Page <dp...@pgadmin.org> wrote: >>>> >>>>> Hi >>>>> >>>>> On Thu, Apr 19, 2018 at 6:56 PM, Anthony Emengo <aeme...@pivotal.io> >>>>> wrote: >>>>> >>>>>> Hey Akshay >>>>>> >>>>>> This patch passed our test pipelines. >>>>>> >>>>> >>>>> Did you test the feature and//or review the code and tests? Passing >>>>> the tests is great, *if* the whole feature is covered (and the nature of >>>>> this patch will make that quite difficult, maybe impossible to do without >>>>> external infrastructure and config). >>>>> >>>> >>>> Agreed, it's been difficult to write test case to test the complete >>>> feature. >>>> >>>>> >>>>> >>>>>> >>>>>> Anthony and Victoria >>>>>> >>>>>> On Thu, Apr 19, 2018 at 1:48 AM, Akshay Joshi < >>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>> >>>>>>> Hi Hackers >>>>>>> >>>>>>> I have implemented the SSH Tunnel support using https://pypi.org/ >>>>>>> project/sshtunnel/ python package. Added "SSH Tunnel" Tab in server >>>>>>> dialog. This implementation supports user name /password and >>>>>>> private/public >>>>>>> key combination with Passphrase to crate SSH Tunnel. I have added >>>>>>> regression test case to add server using SSH Tunnel options. >>>>>>> >>>>>>> The given python package(https://pypi.org/project/sshtunnel/) support >>>>>>> Python version *2.7, 3.4+*. >>>>>>> It uses Paramiko (Python implementation of SSHv2 protocol) which >>>>>>> actually drops support for Python 2.6. So I have added >>>>>>> *SUPPORT_SSH_TUNNEL* parameter in config.py which checks the python >>>>>>> version and set the flag accordingly. In case of Python 2.6, 3.0, 3.1, >>>>>>> 3.2 >>>>>>> and 3.3 control on the "SSH Tunnel" tab of server dialog will be >>>>>>> disabled. >>>>>>> >>>>>>> Please review it, and if looks good please commit the code. >>>>>>> >>>>>>> -- >>>>>>> *Akshay Joshi* >>>>>>> >>>>>>> *Sr. Software Architect * >>>>>>> >>>>>>> >>>>>>> >>>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 >>>>>>> 976-788-8246 <+91%2097678%2088246>* >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Dave Page >>>>> Blog: http://pgsnake.blogspot.com >>>>> Twitter: @pgsnake >>>>> >>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>> The Enterprise PostgreSQL Company >>>>> >>>> >>>> >>>> >>>> -- >>>> *Akshay Joshi* >>>> >>>> *Sr. Software Architect * >>>> >>>> >>>> >>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 >>>> 976-788-8246 <+91%2097678%2088246>* >>>> >>> >>> >> >> >> -- >> *Akshay Joshi* >> >> *Sr. Software Architect * >> >> >> >> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 976-788-8246 >> <+91%2097678%2088246>* >> > -- *Akshay Joshi* *Sr. Software Architect * *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*