On Thu, Mar 29, 2018 at 11:45 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote:
> Hi Rahul, > I see you extracted some code, that is a pretty good move :D > > We run the patch through the testing pipeline and everything is green GJ :D > Also tested the functionality by hand and looks like it is working except > for "add the .sql extension when format is set to SQL." if you set it to > All Files the extension is also added. Not sure if this is a big deal or > not. Lets see what other people think. > Yes, I also think it should append .sql only if the sql extension is selected and user has not provided extension. Let say If I want to save the file with .txt extension then I can use All Files. > Codewise here are some of my comments: > . You added the yarn-error.log file and a migration to the patch doesn't > look intentional. Can you please remove them? > . Also in the patch there are 2 file (moc_LogWindow.cpp and > ui_LogWindow.h) that look like they do not belong to the patch (Did you > rebase your branch before trying to create the patch? > > The test file: test_save_query_to_file.py is empty, it is missing some > tests there. > > As a convention we user lower case names for functions and UpperCase for > class > > Please, regenerate the patch following my previous comments. > > Thanks > Joao > > On Thu, Mar 29, 2018 at 12:54 PM Rahul Soshte <rahulsoshte...@gmail.com> > wrote: > >> Hi, >> When using save or save as feature if .sql is not provided this Patch >> appends it. >> as clearly mentioned in this link. >> >> https://redmine.postgresql.org/issues/1998 >> >> I have ran pep8,regression and Jasmine tests too. >> >> I have primarily changed these files >> web/pgadmin/tools/sqleditor/__init__.py >> web/pgadmin/tools/sqleditor/static/js/sqleditor.js >> web/pgadmin/tools/sqleditor/utils/save_query_to_file.py >> >> >> Regards, >> Rahul Soshte (Hunter) >> >>
