php-mode icon indicating copy to clipboard operation
php-mode copied to clipboard

Proposal for better indentation for SQL strings?

Open ReneFroger opened this issue 9 years ago • 8 comments

Would it be possible to indent on specific keywords of SQL strings in PHP-mode? For example, when I'm typing a SQL string, and press RET on line 2:

public function FooBar() {
       $sSql = 'SELECT foo, bar 
FROM footable 
WHERE foo = 1' 
}

I would like to see have the indentation in PHP-mode like this:

public function FooBar() {
      $sSql = 'SELECT foo, bar
                 FROM footable 
                WHERE foo = 1'
}

Is that even possible?

ReneFroger avatar Nov 05 '15 17:11 ReneFroger

Is that even possible?

Yes, but strictly in the technical sense. I know it can be done, but I also cannot readily think of the best way to implement such indentation. The "obvious" solution that comes to mind is to look at the string, see if each line that composes the string begins with an SQL keyword, and if so indent them accordingly to line-up with the start of the string. But I would be hesitant to go down that road because I don't want PHP Mode trying to determine if strings are SQL and possibly screwing up the indentation of other, non-SQL strings due to mistakes. And I'm leary about implementing support for languages besides PHP within PHP Mode, if only because historically I feel like it's been the source of a lot of problems---I have to tip my hat to Web Mode for doing a great job in that regard.

Personally I have used utility functions like this in the past for situations like this. Admittedly though that specific code still wouldn't indent the string exactly like you want.

I've honestly never looked much at the code for SQL Mode, so maybe there is some functionality within it that we can leverage for this purpose. So I will look into that. I also gladly welcome ideas and suggestions from everyone else. I would prefer approaches that keep PHP Mode ignorant of SQL as much as possible, in the sense of parsing, looking for syntax, etc., so as to ease maintainability. But I'm also not completely opposed to such approaches either; I just cannot think of a satisfactory way myself to add that kind of logic into PHP Mode.

ejmr avatar Nov 06 '15 00:11 ejmr

Thanks for your well written answer.

I understand your thoughts about this completely. However, I would not speaking strictly about finding the SQL keywords for indentation, but looking where the string ends. Perhaps that would make it less difficult?

For example, I have the following:

public function FooBar() {
        $sSql = "string foobar  [Enter here]
foobar string";
}

In order to indent the string properly when a RET follows, you could consider:

public function FooBar() {
     $sSql = "string foobar [Enter here]
              foobar string";
}

Then there is no need to look to the code for SQL mode and it keeps PHP mode ignorant of SQL. Is this a suitable approach?

ReneFroger avatar Nov 06 '15 16:11 ReneFroger

Perhaps an option that, when active (perhaps off by default), will also indent strings, with indentation up to the first character in the string?

This would be very useful. SQL tends to be the only multi-line strings I use when maintaining projects without some kind of query builder - for anything else, I store the text elsewhere.

In such projects, I currently use a very simple utility function to make SQL more readable as a work-around:

function _spaces()
{
    return implode(" ", func_get_args());
}

$statement = $db->prepare(_spaces(
    "SELECT some_fields",
    "FROM some_table",
    "WHERE id = ?"
));

Obviously, this will use PHP indentation so allows for some readability. Quick enough to implement when budget constraints prevent conversion to a decent DB lib.

jwdunne avatar Nov 06 '15 17:11 jwdunne

Thank you both @ReneFroger and @jwdunne for your ideas.

However, I would not speaking strictly about finding the SQL keywords for indentation, but looking where the string ends. Perhaps that would make it less difficult?

Lining up strings regardless of their content in that way would certainly be less difficult. It is (probably) possible to implement in terms of the various CC Mode settings we use for all kinds of other identation; so one possibility is providing an optional style setting for such indentation, or maybe a separate command but I prefer the former. I would be hesitant to make such indentation the default behavior though because PHP Mode doesn't know when whitespace is significant or not in any string. For SQL queries in this use-case, the extra whitespace doesn't hurt anything. But when using strings for anything else it could easily be undesirable, so I wouldn't want PHP Mode to (by default) start treating whitespace in strings as if they're insignificant solely for the purposes of indentation.

In such projects, I currently use a very simple utility function to make SQL more readable as a work-around:

Neat, simple snippet. I know you're using it as a Band Aid, but I think that's clever.

Personally when writing SQL queries in PHP like this I prefer to use the string concatenation operator. I feel like it lines up relatively nicely, although there's room for improvement there still. The only "gotcha" to it that I run into is when forgetting to add a leading/trailing space in each string so the concatenated result doesn't come out as "SELECT some_fieldsFROM some_tableWHERE id = ?". I like how _spaces() avoids that. But I'd like to ask both of you your opinions on using the . operator to combine strings for SQL queries over multiple lines and more specifically why you don't take that approach, because that may illuminate some other indentation issues we should address.

@ReneFroger, I think your proposal sounds the most simple and reasonable, although again it's the kind of indentation/line-up that I wouldn't want to be automatic by default. So I will experiment around with what we can do to achieve that effect. My thanks again to both of you for the great ideas and feedback.

ejmr avatar Nov 06 '15 23:11 ejmr

I actually used the concat operator before I used this. Indentation worked fine. I actually forgot the trailing spaces regularly.

Perhaps using PHP_EOL might be a better separator - the line of error will be reported by the SQL server.

It would be a nice non-default feature!

Sent from my iPhone

On 6 Nov 2015, at 23:18, Eric James Michael Ritz [email protected] wrote:

Thank you both @ReneFroger and @jwdunne for your ideas.

However, I would not speaking strictly about finding the SQL keywords for indentation, but looking where the string ends. Perhaps that would make it less difficult?

Lining up strings regardless of their content in that way would certainly be less difficult. It is (probably) possible to implement in terms of the various CC Mode settings we use for all kinds of other identation; so one possibility is providing an optional style setting for such indentation, or maybe a separate command but I prefer the former. I would be hesitant to make such indentation the default behavior though because PHP Mode doesn't know when whitespace is significant or not in any string. For SQL queries in this use-case, the extra whitespace doesn't hurt anything. But when using strings for anything else it could easily be undesirable, so I wouldn't want PHP Mode to (by default) start treating whitespace in strings as if they're insignificant solely for the purposes of indentation.

In such projects, I currently use a very simple utility function to make SQL more readable as a work-around:

Neat, simple snippet. I know you're using it as a Band Aid, but I think that's clever.

Personally when writing SQL queries in PHP like this I prefer to use the string concatenation operator. I feel like it lines up relatively nicely, although there's room for improvement there still. The only "gotcha" to it that I run into is when forgetting to add a leading/trailing space in each string so the concatenated result doesn't come out as "SELECT some_fieldsFROM some_tableWHERE id = ?". I like how _spaces() avoids that. But I'd like to ask both of you your opinions on using the . operator to combine strings for SQL queries over multiple lines and more specifically why you don't take that approach, because that may illuminate some other indentation issues we should address.

@ReneFroger, I think your proposal sounds the most simple and reasonable, although again it's the kind of indentation/line-up that I wouldn't want to be automatic by default. So I will experiment around with what we can do to achieve that effect. My thanks again to both of you for the great ideas and feedback.

— Reply to this email directly or view it on GitHub.

jwdunne avatar Nov 07 '15 10:11 jwdunne

Perhaps using PHP_EOL might be a better separator - the line of error will be reported by the SQL server.

Good idea, thanks!

ejmr avatar Nov 08 '15 20:11 ejmr

Just a side note, the function in this stack overflow answer seems to work really well to indent the sql string at point: http://stackoverflow.com/a/24659949/6630186

mallt avatar Jul 24 '16 20:07 mallt

Using the align support in https://elpa.gnu.org/packages/sql-indent.html feels like a worthwhile option.

phil-s avatar Oct 16 '18 04:10 phil-s