Use raw literal strings goodness #31031
Conversation
roji
changed the title
| .Append("BEGIN") | ||
| .ToString(); | ||
| return | ||
| $""" |
There was a problem hiding this comment.
Just picking one example in these changes- couldn't this whole string be indented all the way to its more natural position (e.g. one tab space in from the above return) and take advantage of the fact that left whitespace is ignored with raw literal strings?
There was a problem hiding this comment.
It could, but I tend to systematically send multiline SQL strings like this to the very left, to use up all that valuable whitespace (these tend to sometimes be long lines). It's also an embedded language at the end of the day (SQL), so it makes sense to me to read it like that - any particular argument/objection?
There was a problem hiding this comment.
No real objection, I just figured the main advantages of raw string literals were the handling of quotes and whitespace, neither of which seemed taken full advantage of here, so wanted to check your thinking. It's still very helpfully removing that initial whitespace and what you said makes sense, so all good :)
There was a problem hiding this comment.
Sure. It's still useful in that it allows the first line to be aligned the same as the rest (without adding a leading newline), and it also frees us from having to escape double-quotes, which is super-useful for SQLite baselines where quotes appear very often inside SQL...
| @@ -235,7 +248,10 @@ public override void Can_generate_idempotent_up_scripts() | |||
| BEGIN TRANSACTION; | |||
| GO | |||
|
|
|||
| IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'00000000000001_Migration1') | |||
| IF NOT EXISTS ( | |||
There was a problem hiding this comment.
@bricelam note this small formatting change around IF EXISTS/NOT EXISTS - I made this to align it more with the kind of SQL we produce in query, and generally make it a bit more readable. Let me know if you're OK with this.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| var builder = new StringBuilder() | ||
| .Append("IF OBJECT_ID(") | ||
| .Append( | ||
| stringTypeMapping.GenerateSqlLiteral( | ||
| SqlGenerationHelper.DelimitIdentifier(TableName, TableSchema))) | ||
| .AppendLine(") IS NULL") | ||
| .AppendLine("BEGIN"); | ||
| var builder = new StringBuilder( | ||
| $""" | ||
| IF OBJECT_ID({stringTypeMapping.GenerateSqlLiteral(SqlGenerationHelper.DelimitIdentifier(TableName, TableSchema))}) IS NULL | ||
| BEGIN | ||
|
|
||
| """); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Good point, I'll revert this part of the change and keep using Append/AppendLine. We should maybe have a function for taking care of that, i.e. pass a string and making sure its newlines correspond to the runtime platform (but not in this PR).
There was a problem hiding this comment.
...or just normalize all SQL to \n to save bytes 😉
There was a problem hiding this comment.
Right, something or other :) We did have that recent discussion on varying newlines from different cilent machines causing duplicate plan cache entries in SQL Server (though that's really irrelevant to anything in migrations).
Anyway...
No description provided.