MIME-Version: 1.0
References: 
 <CAKyzeV1fMB6rPMF+LCMY7B6_2-j7DZfmh307dBFemtR=4taX6g@mail.gmail.com>
In-Reply-To: 
 <CAKyzeV1fMB6rPMF+LCMY7B6_2-j7DZfmh307dBFemtR=4taX6g@mail.gmail.com>
From: Joao De Almeida Pereira <jdealmeidapereira@pivotal.io>
Date: Thu, 29 Mar 2018 18:15:39 +0000
Message-ID: 
 <CAE+jjam7ks2X=x5WoEp7WEn4tLNzw8Gzu+Mqx1sUm0OUu4Md-A@mail.gmail.com>
Subject: Re: [pgAdmin4][Patch][Feature #1998] Appends .sql if extension not
 given when using 'save' or 'save as' feature
To: Rahul Soshte <rahulsoshte360@gmail.com>
Cc: pgadmin-hackers@postgresql.org
Content-Type: multipart/alternative; boundary="94eb2c05b49e0b50e80568911d9f"
Precedence: bulk

--94eb2c05b49e0b50e80568911d9f
Content-Type: text/plain; charset="UTF-8"

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.

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 <rahulsoshte360@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)
>
>

--94eb2c05b49e0b50e80568911d9f
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hi Rahul,<div>I see you extracted some code, that is a pre=
tty good move :D</div><div><br></div><div>We run the patch through the test=
ing pipeline and everything is green GJ :D</div><div>Also tested the functi=
onality by hand and looks like it is working except for &quot;<span style=
=3D"background-color:rgb(255,255,221);color:rgb(51,51,51);font-family:Verda=
na,sans-serif;font-size:12px">add the .sql extension when format is set to =
SQL.</span>&quot; if you set it to All Files=C2=A0 the extension is also ad=
ded. Not sure if this is a big deal or not. Lets see what other people thin=
k.</div><div><br></div><div>Codewise here are some of my comments:</div><di=
v>. You added the yarn-error.log file and a migration to the patch doesn=
9;t look intentional. Can you please remove them?</div><div>. Also in the p=
atch 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?</div><div><br></div><div>The test file:=C2=A0test_save=
_query_to_file.py is empty, it is missing some tests there.</div><div><br><=
/div><div>As a convention we user lower case names for functions and UpperC=
ase for class</div><div><br></div><div>Please, regenerate the patch followi=
ng my previous comments.</div><div><br></div><div>Thanks</div><div>Joao</di=
v><br><div class=3D"gmail_quote"><div dir=3D"ltr">On Thu, Mar 29, 2018 at 1=
2:54 PM Rahul Soshte &lt;<a href=3D"mailto:rahulsoshte360@gmail.com">rahuls=
oshte360@gmail.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote=
" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><=
div dir=3D"ltr"><div><div><div><div><div>Hi,<br>When using save or save as =
feature if .sql is not provided this Patch appends it.<br>as clearly mentio=
ned in this link.<br><br><a href=3D"https://redmine.postgresql.org/issues/1=
998" target=3D"_blank">https://redmine.postgresql.org/issues/1998</a><br><b=
r></div>I have ran pep8,regression and Jasmine tests too.<br><br></div>I ha=
ve primarily changed these files<br>=C2=A0=C2=A0=C2=A0=C2=A0 web/pgadmin/to=
ols/sqleditor/__init__.py<br>=C2=A0=C2=A0=C2=A0=C2=A0 web/pgadmin/tools/sql=
editor/static/js/sqleditor.js<br>=C2=A0=C2=A0=C2=A0=C2=A0 web/pgadmin/tools=
/sqleditor/utils/save_query_to_file.py<br><br></div><div><br></div>Regards,=
<br></div>Rahul Soshte (Hunter)<br><br></div></div>
</blockquote></div></div>

--94eb2c05b49e0b50e80568911d9f--