Re: PATCH: Format SQL (external tool)

Hi J.F. Oster,

Looks good to me.
Committed to the repository on master branch.

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company
<http://www.enterprisedb.com>

*http://www.linkedin.com/in/asheshvashi*
<http://www.linkedin.com/in/asheshvashi>

On Mon, Jul 20, 2015 at 1:21 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Have you got this Ashesh?
>
> On Sat, Jul 18, 2015 at 1:06 PM, J.F. Oster <jinfroster(at)mail(dot)ru> wrote:
>
>> Hi All,
>>
>>
>> I haven't received any feedback on last message...
>>
>> Attaching a new version of patch. Now it reads a value for timeout from
>> sysSettings, but I didn't add an input to Options form for reasons
>> explained earlier.
>>
>>
>> Please see the attached patch.
>>
>> Thanks.
>>
>>
>>
>>
>> Friday, June 26, 2015, 9:48:52 PM, J.F. Oster wrote:
>>
>>
>> >
>>
>> Hello Ashesh,
>>
>>
>> Sorry for long absense.
>>
>>
>> Are you sure we need that additional option for specifying timeout?
>>
>> How do you see it placed in the Options dialogue? For me it will look
>> somewhat surplus and "overloading" the form.
>>
>> It will be needed to change extremely rarely. The only true case I can
>> imagine is using a wrapper script for online formatting service.
>>
>> I suggest to provide a value in sysSettings, but keep it hidden. If
>> needed, the user will be able to change it by hand. The one who manages to
>> use such advanced functionality IS a hacker already :) and won't feel much
>> inconvenience anyway.
>>
>> How do you think?
>>
>>
>> Regarding the events internals - honestly, it's too complicated for me...
>>
>> The very first version of my code in case of a timeout could lead to
>> SEGFAULT somewhere inside wxWidget's event handling internals, with no
>> pgAdmin's code in the stack.
>>
>> After I've changed by guess the AbortProcess() code to it's current look,
>> it never broke since that. But I can't tell anymore deeper, sorry... :(
>>
>>
>>
>> Wednesday, May 27, 2015, 12:56:25 PM, you wrote:
>>
>>
>> >
>>
>> On Wed, May 27, 2015 at 3:19 PM, Ashesh Vashi <
>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>> Hi J. F. Oster,
>>
>>
>> I think - we should give option to the user about wait timeout (which is
>> hard-coded to 3 seconds).
>>
>> It should be asked in the options dialog.
>>
>> Apart from that - in the AbortProcess function, we're releasing the
>> process object.
>>
>> Does EVT_END_PROCESS event get fired even in case of killing the process
>> across all supported platform?
>>
>> (FYI - I've not tested the code yet.)
>>
>>
>>
>> --
>>
>> Thanks & Regards,
>>
>>
>> Ashesh Vashi
>>
>> EnterpriseDB INDIA: Enterprise PostgreSQL Company
>> <http://www.enterprisedb.com>
>>
>>
>> http://www.linkedin.com/in/asheshvashi
>>
>>
>>
>> On Wed, May 27, 2015 at 10:56 AM, Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>
>>
>>
>> On Tue, May 26, 2015 at 9:51 PM, J.F. Oster <jinfroster(at)mail(dot)ru> wrote:
>>
>>
>> Hello Akshay,
>>
>>
>> Is there something else to fix?
>>
>>
>> Nothing. Patch looks good to me.
>>
>>
>> JFO> Hi Akshay,
>>
>>
>> JFO> Removed that.
>>
>>
>>
>> JFO> Tuesday, May 19, 2015, 10:04:59 AM, you wrote:
>>
>>
>> AJ>> Hi J.F
>>
>>
>> AJ>> The version of fsql you have suggested works for me as well. I
>>
>> AJ>> have reviewed your patch and it looks good to me. Please remove
>>
>> AJ>> the commented code (wxString s; //, tmp at line 873 in
>> sysSettings.cpp.
>>
>>
>>
>> AJ>> On Mon, May 18, 2015 at 10:31 PM, J.F. Oster <jinfroster(at)mail(dot)ru>
>> wrote:
>>
>> AJ>> Hi Akshay,
>>
>>
>> AJ>> fsqlf.exe is the program to use; wx_fsqlf.exe is just a GUI wrapper.
>>
>>
>> AJ>> I've got the latest version (fsqlf.v0.03-292-gd0fd9bf.zip), and it
>> really fails to run
>>
>> AJ>> Please try the previous one, it works for me.
>>
>> AJ>>
>> http://sourceforge.net/projects/fsqlf/files/fsqlf.v0.03/fsqlf.v0.03-141-g94f5a5f.zip.gz/download
>>
>>
>> AJ>> Also please note that fsqlf.exe could fail when run in a path
>> containing national characters.
>>
>>
>>
>>
>> AJ>> Monday, May 18, 2015, 3:42:11 PM, you wrote:
>>
>>
>>
>>
>>
>> AJ>> Hi J.F
>>
>>
>> AJ>> I am reviewing your patch. I have applied the patch and try to
>>
>> AJ>> test it on Windows 7. Below are the steps that I perform
>>
>> AJ>> ° Download SQL Formatter from http://fsqlf.sourceforge.net/
>>
>> AJ>> ° Given the path of fsqlf.exe/wx_fsqlf.exe in File -
>>
>> AJ>> Options - Query Editor: External formatting utility
>>
>> AJ>> ° I have opened the query tool and wrote some select query.
>>
>> AJ>> Please refer the attached screenshot for SQL query.
>>
>> AJ>> When I have given fsqlf.exe in the path it throws the error ( see
>>
>> AJ>> attached screenshot) and when I have given wx_fsqlf.exe in the
>>
>> AJ>> path it always report an error "Formatting command did not respond
>>
>> AJ>> in 3 seconds" in the status bar.
>>
>>
>> AJ>> I am not sure how to test it properly. Can you please provide some
>> steps.
>>
>>
>>
>> AJ>> On Mon, May 18, 2015 at 10:10 AM, Akshay Joshi
>>
>> AJ>> <akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>
>> AJ>> Sure.
>>
>>
>>
>> AJ>> On Fri, May 15, 2015 at 9:30 PM, Dave Page <dpage(at)pgadmin(dot)org>
>> wrote:
>>
>>
>> AJ>> Akshay, can you take a look please?
>>
>>
>> AJ>> Thanks.
>>
>>
>>
>> AJ>> On Fri, May 15, 2015 at 4:53 PM, J.F. Oster <jinfroster(at)mail(dot)ru>
>> wrote:
>>
>> >>> Hello!
>>
>>
>> >>> Please take a look at the patch.
>>
>> >>> Thanks.
>>
>>
>> >>> Per discussion
>>
>> >>>
>> http://www.postgresql.org/message-id/CAPyomk5NT9Tm-r3wombLzoY60Vqa+QyRDy4u84_2K9UWLbWHTg@mail.gmail.com
>>
>>
>> >>> It's most useful for making readable queries generated by ORMs such as
>>
>> >>> Hibernate. But in general, external processing can go far beyond
>>
>> >>> formatting task.
>>
>>
>> >>> I've implemented this feature quick-and-dirty long ago. Finally I made
>>
>> >>> myself clean it up, now it looks better, so please consider a patch.
>>
>> >>> Tested on Windows 7 and Ubuntu 14.04.
>>
>>
>> >>> Changes:
>>
>> >>> * added new setting, ExtFormatCmd, "External formatting utility" in
>>
>> >>> Options dialogue
>>
>> >>> * added menu item "Edit - Format - External Format" in
>>
>> >>> Query editor
>>
>> >>> * class sysProcess supports UTF-8 and can pass STDIN for a process.
>>
>>
>> >>> Suggested use scenario:
>>
>> >>> 1. Download and install some SQL formatting utility.
>>
>> >>> 2. Tell pgAdmin where it resides:
>>
>> >>> File - Options - Query Editor: External formatting utility.
>>
>> >>> 3. Open Query editor. Select a text block to format and press
>>
>> >>> Ctrl-Shift-F. With no selection the whole text gets formatted.
>>
>> >>> In case of non-zero exit code, STDERR will be shown in status bar.
>>
>>
>> >>> Requirements for external formatting utility:
>>
>> >>> * Accepts a STDIN stream and writes result to STDOUT
>>
>> >>> * Finishes in less than 3 seconds
>>
>> >>> * Exits with code 0 on success
>>
>> >>> Support for UTF-8 multibyte characters is preferable.
>>
>>
>> >>> To see whether it works well, a test can be done:
>>
>> >>> C:\> type in.sql |some_formatter >out.sql
>>
>> >>> C:\> echo %ERRORLEVEL%
>>
>> >>> or
>>
>> >>> user(at)linux:~$ cat in.sql |some_formatter >out.sql
>>
>> >>> user(at)linux:~$ echo $?
>>
>>
>> >>> There are few available utilities depending on platform:
>>
>> >>> * Free SQL Formatter (Linux, Windows, Mac OS X(?))
>>
>> >>> http://fsqlf.sourceforge.net/
>>
>> >>> * Poor Man's T-SQL Formatter (Windows)
>>
>> >>> http://architectshack.com/PoorMansTSqlFormatter.ashx
>>
>> >>> Also it is possible to make a wrapper script for numerous online
>>
>> >>> formatting services, but it's less secure and less reliable.
>>
>>
>> >>> Fsqlf is FOSS and seems promising. I think of extending it for
>>
>> >>> PosgreSQL-specific SQL syntax and probably even PL/pgSQL.
>>
>>
>>
>>
>>
>>
>> --
>>
>> Best regards,
>>
>> J.F.
>>
>>
>>
>>
>>
>>
>> --
>>
>>
>> Akshay Joshi
>>
>> Principal Software Engineer
>>
>>
>>
>> Phone: +91 20-3058-9517
>>
>> Mobile: +91 976-788-8246
>>
>>
>>
>>
>>
>> --
>>
>> Best regards,
>>
>> J.F.
>>
>>
>>
>>
>>
>> --
>>
>> Best regards,
>>
>> J.F.
>>
>>
>> --
>> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>