Re: WIP: SQL Formatter

Hi Dave,

On Thu, Jul 30, 2020 at 8:22 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:

>
>
> On Thu, Jul 30, 2020 at 10:23 AM Aditya Toshniwal <
> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>
>>
>>
>> On Thu, Jul 30, 2020 at 2:47 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>>
>>>
>>> On Thu, Jul 30, 2020 at 9:53 AM Aditya Toshniwal <
>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi,
>>>>
>>>> On Thu, Jul 30, 2020 at 1:54 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Thu, Jul 30, 2020 at 6:29 AM Aditya Toshniwal <
>>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi Dave,
>>>>>>
>>>>>> On Wed, Jul 29, 2020 at 9:34 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>> The attached WIP patch adds a menu option to the Query Tool to
>>>>>>> format the SQL in the editor. It does so per options that can be set in the
>>>>>>> Preferences panel (essentially, most of these:
>>>>>>> https://sqlparse.readthedocs.io/en/latest/api/#formatting-of-sql-statements
>>>>>>> )
>>>>>>>
>>>>>>> Some thoughts before I continue:
>>>>>>>
>>>>>>> - There are already options for tabs vs spaces and tab width for the
>>>>>>> query tool. At the moment I've intentionally kept separate settings for the
>>>>>>> same thing in the formatter. If we use the same options it'll mean that
>>>>>>> configuration for formatting is split across two places in the Preferences
>>>>>>> panel. On the other hand, it may be handy to have separate options. What do
>>>>>>> others think?
>>>>>>>
>>>>>> I'm not aware of any editor who is having separate settings for
>>>>>> formatting. Editors like VS code use .editorconfig for the auto format
>>>>>> option. I would also suggest having common editor settings for both
>>>>>> formatting and user input.
>>>>>>
>>>>>
>>>>> Fair point. Should we move the indent options into the SQL Formatting
>>>>> section of the Preferences then? That seems like it could get confusing as
>>>>> they'd then be separated from other settings related to formatting that are
>>>>> specific to the editor.
>>>>>
>>>> In pgAdmin, all the SQL view(editor, sql tab) related settings are
>>>> taken from Query tool -> Editor currently. I suggest we can add a new node
>>>> - Autoformat parallel to Editor and put only autoformat related settings in
>>>> there. It is understood where the other settings like tab space, etc.
>>>> comes from. What do you think ? Currently there is no way to display groups
>>>> in preferences, otherwise auto format would go in the editor node but in a
>>>> group named Autoformat.
>>>>
>>>
>>> Well we have subsections. Currently the patch makes it look like the
>>> following. How would you suggest we change it - remove the indent options
>>> from the Query Tool -> Editor section and only have them in the Formatting
>>> -> SQL section?
>>>
>> I suggest remove node Formatting and move Formatting->SQL to Query tool
>> with the name "Auto-format", just like Auto complete. And yes, remove
>> the options which are already there in Editor.
>>
>
> My concern with that (which I've always had with the other formatting
> options that affect CodeMirror directly) is that they are not related
> *only* to the Query Tool; in this case, in the future it may affect resql
> as well.
>
> Maybe the answer is to go the other way, and move everything under
> Formatting?
>
Currently, all the CodeMirrors in pgAdmin use the settings from the Query
tool -> Editor, even though they're not related. I think you're right. We
should move all CodeMirror settings out of the Query tool and move it to a
new node - "SQL" may be. Editor and Auto-format would be sub nodes under
SQL. With that, the editor settings will apply at all the places where
CodeMirror editing is used(function body, query tool). What do you say ?

>
>
>>
>>> [image: Screenshot 2020-07-30 at 10.14.31.png]
>>>
>>> [image: Screenshot 2020-07-30 at 10.14.50.png]
>>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> - I'm thinking that maybe we should push all user-visible generated
>>>>>>> SQL through the formatter. This would essentially mean that all get_sql and
>>>>>>> similar functions would call it. We'd probably need to make the re-sql test
>>>>>>> suite call it as well. Does this seem like a good idea? It's be a fairly
>>>>>>> widespread change, but it would mean that the resql and generated crud
>>>>>>> statements would be consistently formatted, to the user's preferences.
>>>>>>>
>>>>>> Yes we can. But should we use it for function/proc body ? Users may
>>>>>> not like their function being altered.
>>>>>>
>>>>>
>>>>> Also a fair point. I wonder if we can just format the outer
>>>>> boilerplate first, then inject the body.
>>>>>
>>>> Yes. That can be done.
>>>>
>>>>>
>>>>> Either way, that part would be a phase two project. I'm planning to
>>>>> get the basic formatter in first.
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> --
>>>>>>> Dave Page
>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>> Twitter: @pgsnake
>>>>>>>
>>>>>>> EDB: http://www.enterprisedb.com
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Thanks,
>>>>>> Aditya Toshniwal
>>>>>> pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com*
>>>>>> <http://edbpostgres.com>
>>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> Blog: http://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>> EDB: http://www.enterprisedb.com
>>>>>
>>>>>
>>>>
>>>> --
>>>> Thanks,
>>>> Aditya Toshniwal
>>>> pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com*
>>>> <http://edbpostgres.com>
>>>> "Don't Complain about Heat, Plant a TREE"
>>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EDB: http://www.enterprisedb.com
>>>
>>>
>>
>> --
>> Thanks,
>> Aditya Toshniwal
>> pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com*
>> <http://edbpostgres.com>
>> "Don't Complain about Heat, Plant a TREE"
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EDB: http://www.enterprisedb.com
>
>

--
Thanks,
Aditya Toshniwal
pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com*
<http://edbpostgres.com>
"Don't Complain about Heat, Plant a TREE"