syntax based indentation for SQL files (ELPA package proposal)

syntax based indentation for SQL files (ELPA package proposal)

[Alex Harsanyi]

Hello,

I wrote a package that adds support for syntax based indentation for
SQL files: TAB indents the current line based on the syntax of the SQL
code on previous lines.  This works similar to the indentation for
cc-mode.  The current sql-mode does not provide indentation
functionality, and my package implements a minor mode that can be
added to sql-mode to provide syntactic indentation.

I'd like to propose to include the package into ELPA

You can find the source of the package here:

     https://github.com/alex-hhh/emacs-sql-indent

I already posted twice on this mailing list about this package (see
links below).   While the response seemed positive to me, the package
was not added to ELPA but I also I did not receive a definite "not
interested" answer.  I after some thought, I decided to post about
this one last time.

    https://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00075.html
    https://lists.gnu.org/archive/html/emacs-devel/2015-12/msg00943.html

Best Regards,
Alex.

Re: syntax based indentation for SQL files (ELPA package proposal)

[Stefan Monnier]

> I already posted twice on this mailing list about this package (see
> links below).   While the response seemed positive to me, the package
> was not added to ELPA but I also I did not receive a definite "not
> interested" answer.  I after some thought, I decided to post about
> this one last time.

>     https://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00075.html
>     https://lists.gnu.org/archive/html/emacs-devel/2015-12/msg00943.html

I think this is very interesting and should either make it into
sql-mode.el or GNU ELPA (or both).  I'll be happy to help, regardless of
the option you choose.


        Stefan

Re: syntax based indentation for SQL files (ELPA package proposal)

[Steinar Bang]

>>>>> Stefan Monnier <monnier@iro.umontreal.ca>:

> I think this is very interesting and should either make it into
> sql-mode.el or GNU ELPA (or both).  I'll be happy to help, regardless
> of the option you choose.

(FWIW I've felt that the lack of indentation support in the current SQL
mode is unexpected behaviour, so I think that inclusion into sql-mode.el
would be the best thing from a user perspective.)

Re: syntax based indentation for SQL files (ELPA package proposal)

[Alex Harsanyi]

On Sun, Aug 6, 2017 at 12:05 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> I already posted twice on this mailing list about this package (see
>> links below).   While the response seemed positive to me, the package
>> was not added to ELPA but I also I did not receive a definite "not
>> interested" answer.  I after some thought, I decided to post about
>> this one last time.
>
>>     https://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00075.html
>>     https://lists.gnu.org/archive/html/emacs-devel/2015-12/msg00943.html
>
> I think this is very interesting and should either make it into
> sql-mode.el or GNU ELPA (or both).  I'll be happy to help, regardless of
> the option you choose.
>

Hi Stefan,

I'm not sure what are the next steps for this?

I already have signed copyright assignment for GNU Emacs contributions
in the past.  The package has another contributor, Pierre Téchoueyres,
I'm not sure if he has signed the copyright assignment papers or not
(I just sent him an email asking this).

I don't have commit privileges for either the ELPA or the GNU Emacs
sources, so I cannot add the package myself.

I think this should be a separate package, at least initially.  Based
on feedback from Michael who maintains sql.el, there might be people
who prefer the way sql-mode works now, as it has been like this for a
long time.

Best Regards,
Alex.

Re: syntax based indentation for SQL files (ELPA package proposal)

[Stefan Monnier]

> I'm not sure what are the next steps for this?

I think the main question is whether it can/should be included in
sql.el.  Maybe the best strategy is the following:

- add sqlindent.el more-or-less as-is into Emacs's `master` branch as
  lisp/progmodes/sqlindent.el
- change sql.el to use that indentation package by default.

Michael, what do you say?  Do you want to include sqlindent.el directly
into sql.el?

> I already have signed copyright assignment for GNU Emacs contributions
> in the past.  The package has another contributor, Pierre Téchoueyres,
> I'm not sure if he has signed the copyright assignment papers or not
> (I just sent him an email asking this).

Yes, he has already signed paperwork for Emacs.

> I think this should be a separate package, at least initially.  Based
> on feedback from Michael who maintains sql.el, there might be people
> who prefer the way sql-mode works now, as it has been like this for a
> long time.

I believe the default behavior should be to enable automatic
indentation.  We can provide some way for the user to disable it, of
course (actually, I think it would make sense to offer a generic minor
mode which provides "dumb" indentation in any major mode).


        Stefan

Re: syntax based indentation for SQL files (ELPA package proposal)

[Stefan Monnier]

> Totally in agreement: include as a separate file and have sql.el use it for indenting by default.

OK.
Should we include it in GNU ELPA (and make sql.el use it only when
available), or should we include it in Emacs?


        Stefan

Re: syntax based indentation for SQL files (ELPA package proposal)

[John Wiegley]

>>>>> "SM" == Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

SM> OK. Should we include it in GNU ELPA (and make sql.el use it only when
SM> available), or should we include it in Emacs?

I would prefer ELPA.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: syntax based indentation for SQL files (ELPA package proposal)

[simenheg]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> I believe the default behavior should be to enable automatic
> indentation.  We can provide some way for the user to disable it, of
> course (actually, I think it would make sense to offer a generic minor
> mode which provides "dumb" indentation in any major mode).

I'm happy to see that sql-indent was added to ELPA last year, but I
agree with Stefan that it would be even better if SQL mode offered
automatic indentation by default, like major modes usually do.

I could make an attempt to integrate it with SQL mode, with an option to
turn it off, if there is interest for it?

-- Simen

Re: syntax based indentation for SQL files (ELPA package proposal)

[Stefan Monnier]

>> I believe the default behavior should be to enable automatic
>> indentation.  We can provide some way for the user to disable it, of
>> course (actually, I think it would make sense to offer a generic minor
>> mode which provides "dumb" indentation in any major mode).
> I'm happy to see that sql-indent was added to ELPA last year, but I
> agree with Stefan that it would be even better if SQL mode offered
> automatic indentation by default, like major modes usually do.
>
> I could make an attempt to integrate it with SQL mode, with an option to
> turn it off, if there is interest for it?

The main prerequisite for that would be to move sql-indent from elpa.git
to emacs.git.  I think it's a good idea to bring sql.el and
sql-indent.el closer to each other (an alternative would be to move
sql.el to elpa.git, but that's probably not gonna fly), but it's rather
a question for Alex, Michael, and Emacs's maintainers.


        Stefan

Re: syntax based indentation for SQL files (ELPA package proposal)

[Michael Mauger]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, 11 September 2018 08:16, Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

> >> I believe the default behavior should be to enable automatic
> > > indentation. We can provide some way for the user to disable it, of
> > > course (actually, I think it would make sense to offer a generic minor
> > > mode which provides "dumb" indentation in any major mode).
> > > I'm happy to see that sql-indent was added to ELPA last year, but I
> > > agree with Stefan that it would be even better if SQL mode offered
> > > automatic indentation by default, like major modes usually do.
> >
> > I could make an attempt to integrate it with SQL mode, with an option to
> > turn it off, if there is interest for it?
>
> The main prerequisite for that would be to move sql-indent from elpa.git
> to emacs.git. I think it's a good idea to bring sql.el and
> sql-indent.el closer to each other (an alternative would be to move
> sql.el to elpa.git, but that's probably not gonna fly), but it's rather
> a question for Alex, Michael, and Emacs's maintainers.
>
> Stefan

Apologize for the delay... My thought was that we can add default support
with the two modules in different repositories (although I have no concern
about either proposal made by Stefan: sql, sql-indent both to emacs.git or
to elpa.git). Below is an initial attempt to perform the integration that
could be added to sql.el. In addition, the initial values of `sql-mode-hook'
and `sql-interactive-mode-hook' would be set to '(sql-indent-enable).

Code (un-tested)::

    ;; SQL indent support
    (defcustom sql-use-indent-support t
      "If non-nil then use the SQL indent support features of sql-indent.
    The package must be available to be loaded and activated.")

    (defun sql-is-indent-available ()
      "Check if sql-indent module is available."
      (when (locate-library "sql-indent")
        (require 'sql-indent)
        (fboundp 'sqlind-minor-mode)))

    (defun sql-indent-enable ()
      "Enable `sqlind-minor-mode' if available and requested."
      (when (sql-is-indent-available)
        (sqlind-minor-mode (if sql-use-indent-support +1 -1))))

I'd also recommend we add a large comment block to sql.el recommending
the installation of sql-indent if not installed by default.

--
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer

Re: syntax based indentation for SQL files (ELPA package proposal)

[Stefan Monnier]

>     (defun sql-is-indent-available ()
>       "Check if sql-indent module is available."
>       (when (locate-library "sql-indent")
>         (require 'sql-indent)
>         (fboundp 'sqlind-minor-mode)))

I'd recommend you just use (fboundp 'sqlind-minor-mode) since the ELPA
package (or any proper manual install) sets up an autoload for it.


        Stefan

Re: syntax based indentation for SQL files (ELPA package proposal)

[Michael Mauger]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Sunday, September 16, 2018 5:30 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> > (defun sql-is-indent-available ()
>
> >       "Check if sql-indent module is available."
> >       (when (locate-library "sql-indent")
> >         (require 'sql-indent)
> >         (fboundp 'sqlind-minor-mode)))
> >
>
> I'd recommend you just use (fboundp 'sqlind-minor-mode) since the ELPA
> package (or any proper manual install) sets up an autoload for it.
>
> Stefan

I have pushed a change to sql.el to enable sqind-minor-mode if it is available.

Alex, you may want to look into making a change to sql-indent to make sure users
who have enabled indent mode previously by calling the minor mode or setup function
may end up toggling off indent mode because sql.el has already turned on.

You can check whether `sql-indent-enable' is included in the mode hook list, and
make the minor enabled if `sql-use-indent-support' is non-nil.

Let me know your thoughts.

--
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer

Re: syntax based indentation for SQL files (ELPA package proposal)

[Stefan Monnier]

> Alex, you may want to look into making a change to sql-indent to make
> sure users who have enabled indent mode previously by calling the
> minor mode or setup function may end up toggling off indent mode
> because sql.el has already turned on.

I don't understand what you mean by that.

Calling (sql-indent-mode) (or adding it to sql-mode-hook) will never turn
the mode off: you'd have to call sql-indent-mode interactively or pass
it an explicit -1 argument to turn it off.


        Stefan

Re: syntax based indentation for SQL files (ELPA package proposal)

[Andy Moreton]

On Mon 01 Oct 2018, Michael Mauger wrote:

> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Sunday, September 16, 2018 5:30 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
>> > (defun sql-is-indent-available ()
>>
>> >       "Check if sql-indent module is available."
>> >       (when (locate-library "sql-indent")
>> >         (require 'sql-indent)
>> >         (fboundp 'sqlind-minor-mode)))
>> >
>>
>> I'd recommend you just use (fboundp 'sqlind-minor-mode) since the ELPA
>> package (or any proper manual install) sets up an autoload for it.
>>
>> Stefan
>
> I have pushed a change to sql.el to enable sqind-minor-mode if it is available.

This patch looks like it has been applied in the wrong place. Emacs
builtin code should not depend on an external package. Why did you add
this change into emacs, rather than the package that defines
sqind-minor-mode ?

    AndyM

Re: syntax based indentation for SQL files (ELPA package proposal)

[Stefan Monnier]

> This patch looks like it has been applied in the wrong place.  Emacs
> builtin code should not depend on an external package.  Why did you add
> this change into emacs, rather than the package that defines
> sqind-minor-mode ?

It doesn't actually depend on sql-indent.  It just makes use of it
when available.  Given that sql-indent is in GNU ELPA, I think this is
perfectly acceptable (and I'm pretty sure we have other similar cases
already in emacs.git, some of which for packages that aren't even in
GNU ELPA).


        Stefan

Re: syntax based indentation for SQL files (ELPA package proposal)

[Andy Moreton]

On Tue 02 Oct 2018, Stefan Monnier wrote:

>> This patch looks like it has been applied in the wrong place.  Emacs
>> builtin code should not depend on an external package.  Why did you add
>> this change into emacs, rather than the package that defines
>> sqind-minor-mode ?
>
> It doesn't actually depend on sql-indent.  It just makes use of it
> when available.  Given that sql-indent is in GNU ELPA, I think this is
> perfectly acceptable (and I'm pretty sure we have other similar cases
> already in emacs.git, some of which for packages that aren't even in
> GNU ELPA).

This is still the wrong way around though: the customisation is only
needed by users of sqind-minor-mode, and otherwise adds bloat (and
possibly bugs) to emacs core, for no benefit to any other users.

This approach is misguided. The customisation to support the GNU ELPA
package should be in the GNU ELPA package, not in emacs core.

    AndyM

Re: syntax based indentation for SQL files (ELPA package proposal)

[Thomas Fitzsimmons]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> This patch looks like it has been applied in the wrong place.  Emacs
>> builtin code should not depend on an external package.  Why did you add
>> this change into emacs, rather than the package that defines
>> sqind-minor-mode ?
>
> It doesn't actually depend on sql-indent.  It just makes use of it
> when available.  Given that sql-indent is in GNU ELPA, I think this is
> perfectly acceptable (and I'm pretty sure we have other similar cases
> already in emacs.git, some of which for packages that aren't even in
> GNU ELPA).

Yes, EUDC's optional dependency on BBDB (which is in GNU ELPA now) is
another example.  I don't love it, but I've learned to live with it,
especially given that the BBDB maintainer prefers the current
arrangement.  At one point there was talk of resolving these
dependencies during the creation of Emacs release tarballs.

Thomas

Re: syntax based indentation for SQL files (ELPA package proposal)

[Van L]

> This is still the wrong way around though: the customisation is only
> needed by users of sqind-minor-mode, and otherwise adds bloat (and
> possibly bugs) to emacs core, for no benefit to any other users.

The emacs core should fine tune and make weight before release 
like a UFC 229 prize fighter before release championship fight.

Re: syntax based indentation for SQL files (ELPA package proposal)

[Stefan Monnier]

> This is still the wrong way around though:

The other way is a bit less clean, technically, IMO.

> This approach is misguided. The customisation to support the GNU ELPA
> package should be in the GNU ELPA package, not in emacs core.

The separation between the two is very thin, so it's not that big of
a deal.  It would be better to bring sql.el and sql-indent.el together,
I think, but for various reasons, this is currently not how it is.


        Stefan