Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f1rT8-0003YT-5q for pgadmin-hackers@arkaria.postgresql.org; Fri, 30 Mar 2018 10:41:14 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f1rT6-00069c-E6 for pgadmin-hackers@arkaria.postgresql.org; Fri, 30 Mar 2018 10:41:12 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f1rT6-00069S-1Q for pgadmin-hackers@lists.postgresql.org; Fri, 30 Mar 2018 10:41:12 +0000 Received: from mail-ot0-x236.google.com ([2607:f8b0:4003:c0f::236]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1f1rT2-0001EA-OT for pgadmin-hackers@postgresql.org; Fri, 30 Mar 2018 10:41:10 +0000 Received: by mail-ot0-x236.google.com with SMTP id o9-v6so9187047otj.5 for ; Fri, 30 Mar 2018 03:41:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=zYhrYYz05Lw57XXyTpKR7Q1L3jeDf/UfnQdWtuNb/Vw=; b=vwI1XhsXF5NqrS5AK280tIsJu/1Cpxg8sJoUEKyHFYdiDwQHfERzdW8ylB9jCcxOQU IApN9iHDelsR/KmBsqPUbNtsmbSJr/jbVoRXTmqtjdD6tjiCeadeTgIKFzM/Hmisz5I8 DFWiE0j3IAwSbqq+sN2f1F2N6y8ZoECex2wVozO0polWWHAVadNHSVRsCUUds7I7ofC8 LACzDzCAMVYVdg6A/TFrCy2JQADPUab0IXIUGEsOV0l1FcbrtMt+Ev0zXpae4zTNMxEA /IqBN9B3pewvtrizjWve+/cRXjCc7uVGpV3x2eu03prAgIfXhW8sT4H+C8mhajw+qmaz 5Anw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=zYhrYYz05Lw57XXyTpKR7Q1L3jeDf/UfnQdWtuNb/Vw=; b=d4AyobmiqGsJNDEHVw+hTUtdeyyaRS/qIklqa2SXq9YVZj3r5nnQk9rXBynLeCzH2w 30WYAhQ5iCSwuSC6lOH5KBlW2gkRRervFJLpNYNtiRlUgppJC/Psq9oAXim0IBya6Esm 69i2CUDcXqoaFe0CcU9cHAEJ5VMgQTckAESnWKRIWHV11kc0X1yIh2Y0Zpcu2+0mVycf moW9ne9PWLZ8R+6aibVvRAtzwlqNpw3lz+rtSQIq8iQmPi7OuwzXiEGWfXf8h7btzIe7 SgQiBvwk2qjnQmr8pcZ6tOA4KrAFZsRbi9x4b/TsMGDR06GwNKfVvTlXyps5Elj+JXO0 Ks3Q== X-Gm-Message-State: AElRT7H1dUpIAN6YBs70uciVhc4m8h+PuvXNuVCK0a846OStZuHzRNlm 5zLiF9lEguFFrKB2JoSmo8DDbkZU/rLMt7YSKYqd0A== X-Google-Smtp-Source: AIpwx4+J4CnEx8YxKm43a9SsciayC0l+pT2aDqzCkBmjj1oc9+gvQaUjG4jG/t2pFYcCFwvwEFqRtYaU99+9N05rOoE= X-Received: by 2002:a9d:4e16:: with SMTP id p22-v6mr7381123otf.311.1522406467450; Fri, 30 Mar 2018 03:41:07 -0700 (PDT) MIME-Version: 1.0 Received: by 10.138.6.138 with HTTP; Fri, 30 Mar 2018 03:40:47 -0700 (PDT) In-Reply-To: References: From: Murtuza Zabuawala Date: Fri, 30 Mar 2018 16:10:47 +0530 Message-ID: Subject: Re: [pgAdmin4][Patch][Feature #1998] Appends .sql if extension not given when using 'save' or 'save as' feature To: Joao De Almeida Pereira Cc: Rahul Soshte , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000b24b7e05689ee010" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000b24b7e05689ee010 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Mar 29, 2018 at 11:45 PM, Joao De Almeida Pereira < jdealmeidapereira@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. > =E2=80=8BYes, I also think it should append .sql only if the sql extension = is selected and user has not provided extension.=E2=80=8B =E2=80=8BLet say If I want to save the file with .txt extension then I can = use All Files. =E2=80=8B > 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 > 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) >> >> --000000000000b24b7e05689ee010 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Thu, Mar 29, 2018 at 11:45 PM, Joao De Al= meida Pereira <jdealmeidapereira@pivotal.io> wrot= e:
Hi Rahul,
I see y= ou extracted some code, that is a pretty good move :D

<= div>We run the patch through the testing pipeline and everything is green G= J :D
Also tested the functionality by hand and looks like it is w= orking except for "add the .s= ql extension when format is set to SQL." if you set it to All F= iles=C2=A0 the extension is also added. Not sure if this is a big deal or n= ot. Lets see what other people think.
=E2=80=8BYes, I also think it should append .sql only if the sql extens= ion is selected and user has not provided extension.=E2=80=8B
=C2=A0
=E2=80=8BLet say If I want to save the file with .txt extension then = I can use All Files. =E2=80=8B


Codewise here are some of my comment= s:
. You added the yarn-error.log file and a migration to the pat= ch doesn't look intentional. Can you please remove them?
. Al= so in the patch there are 2 file (moc_LogWindow.cpp and ui_LogWindow.h) tha= t look like they do not belong to the patch (Did you rebase your branch bef= ore trying to create the patch?

The test file:=C2= =A0test_save_query_to_file.py is empty, it is missing some tests there= .

As a convention we user lower case names for fun= ctions 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 Pat= ch appends it.
as clearly mentioned in this link.

https://redmine.p= ostgresql.org/issues/1998

I have ran pep8,regression = and Jasmine tests too.

I have primarily changed these files=C2=A0=C2=A0=C2=A0=C2=A0 web/pgadmin/tools/sqleditor/__init__.py
= =C2=A0=C2=A0=C2=A0=C2=A0 web/pgadmin/tools/sqleditor/static/js/sqledit= or.js
=C2=A0=C2=A0=C2=A0=C2=A0 web/pgadmin/tools/sqleditor/utils/sa= ve_query_to_file.py


Regards,
Rahul Sos= hte (Hunter)


--000000000000b24b7e05689ee010--