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

Properly Indent Implicit Inner Joins #64

Closed kevinjfoley closed 6 years ago

kevinjfoley commented 6 years ago

Currently implicit inner joins (just JOIN) isn't treated the same as an explicit INNER JOIN.

Adding a begging of line anchor to sqlind-select-join-regexp as demonstrated below seems to resolve the issue. I typed out the string as I don't believe there is a way to use an anchor with regexp-opt.

I can try to create some tests for this and open a PR later today but wanted to make sure there wasn't a reason implicit inner joins weren't already accounted for.

(defconst sqlind-select-join-regexp
  (concat "\\b"
      "\\(cross\\|inner\\|left\\|natural\\|right\\|^\\)"
      "[ \t\r\n\f]*join"
      "\\b"))
alex-hhh commented 6 years ago

Hi @kevinjfoley , your fix is not correct, as it will match the white-space before the JOIN, and will create an incorrect anchor point. The correct fix is to put the optional keywords (CROSS, INNER, etc) and the white-space following them in an optional group.

I will only be able to accept a pull request if you have signed copyright assignment papers for GNU Emacs, since this package is part of GNU ELPA and these are the FSF rules (this is detailed in CONTRIBUTING.md file.

Otherwise, I will fix this myself in the next few days.

kevinjfoley commented 6 years ago

Yeah you're right that creates a lot of issues.

I believe this should work but for some reason it sets the anchor on the J in

LEFT JOIN a
ON a = b

I've tested it in re-builder and it seems to work properly but there's probably something I'm missing. Is it because there's no an extra capture group? Sorry I haven't had a chance to dig into the source code to check yet.

Also I don't have signed copyright papers yet so I won't be able to contribute unfortunately, I'll look into getting them.

alex-hhh commented 6 years ago

OK, I fixed this myself, it was a bit more complex than I thought, as the regexp will always match the shorter string when the other keywords are optional, so it would always match "JOIN" even if "INNER JOIN" was present.

Let me know if you have any problems