alex-hhh / emacs-sql-indent

Syntax based indentation for SQL files inside GNU Emacs
GNU General Public License v3.0
121 stars 18 forks source link

Bad indentation for CREATE TRIGGER FOR ROW #88

Closed sfllaw closed 4 years ago

sfllaw commented 4 years ago

Test case for PostgreSQL:

CREATE OR REPLACE FUNCTION s.noop()
  RETURNS TRIGGER AS $$
  BEGIN
    RETURN NEW;
  END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER t
  AFTER INSERT OR UPDATE
  ON tbl
  FOR EACH ROW
    EXECUTE PROCEDURE s.noop();
          |

The cursor is indented to | because there is no special handling in: https://github.com/alex-hhh/emacs-sql-indent/blob/7f649aa97eba2836f8cb1106fb3b01e59cfc71b9/sql-indent.el#L305-L348

I recognize that this is not standard SQL, so maybe you don’t want to put this in. However, the for statement isn’t standard SQL either, so I think it makes sense to handle this case.

According to the CREATE TRIGGER documentation, this can be written in the following ways:

alex-hhh commented 4 years ago

I cannot reproduce this issue, the example code you provided is indenting correctly. I also extended your example to cover all the "for..." variations you listed, and they work correctly:

-- -*- mode: sql; sql-product: postgres; -*-
create or replace function noop()
  returns trigger as $$
  begin
    return new;
  end;
$$ language plpgsql;

create trigger t
  after insert or update
  on tbl
  for row
    execute procedure noop();

create trigger t
  after insert or update
  on tbl
  for each row
    execute procedure noop();

create trigger t
  after insert or update
  on tbl
  for statement
    execute procedure noop();

create trigger t
  after insert or update
  on tbl
  for each statement
    execute procedure noop();
sfllaw commented 4 years ago

I think I made a mistake when creating a minimal test case. You are right that sqlind does the correct thing when it is execute procedure noop(). However, it fails if the function has a fully-qualified name like s.noop().

I have updated the test case in this bug’s description.

It looks like sqlind thinks this is a (defun-start "s") from inside sqlind-maybe-defun-statement. I suspect the bug lies in the regular expression https://github.com/alex-hhh/emacs-sql-indent/blob/b6940780678e95af245830dbcd897672ca0a0a4c/sql-indent.el#L796, which might need to be changed to:

(when (looking-at "\\(procedure\\|function\\)\\(?:[ \t\n\r\f]+\\)\\(\\(?:[a-z0-9_]+\\.\\)?[a-z0-9_]+\\)")
                                                                  ; ~~~~~~~~~~~~~~~~~~~~~~

Note the \\(?:[a-z0-9_]+\\.\\)?, which is a optional, non-capturing group for matching the schema.

sfllaw commented 4 years ago

I think you might have introduced a bug in https://github.com/alex-hhh/emacs-sql-indent/commit/4a85440a88d124ecf823b9bcf8c2687329dfb52d#r36956386?

alex-hhh commented 4 years ago

fixed and merged to master