Added possibility to read non escapable sequences #34
Conversation
|
Coucou! At least a little unit tests? |
|
|
||
| lastQuote = nextQuote + 1; | ||
| } | ||
| while(Cursor.Buffer.Length > lastQuote && Cursor.Buffer[lastQuote] == endSequenceChar); |
There was a problem hiding this comment.
Infinite loop, or if not then it should be a single if. I don't see any state changing in the loop.
There was a problem hiding this comment.
the purpose is to catch multiple cases, so it has to be a while. It would be an infinite loop if I did not have a break on line 511.
There was a problem hiding this comment.
There is a ; at the end of the while line. No?
There was a problem hiding this comment.
it's a do/while, not a while/do ;)
| [InlineData("'Lorem \n ipsum'", "'Lorem \n ipsum'")] | ||
| [InlineData("'Lorem '' ipsum'", "'Lorem '' ipsum'")] | ||
| [InlineData(@"""Lorem """" ipsum""", "\"Lorem \"\" ipsum\"")] | ||
| public void ShouldReadNonEscapableString(string text, string expected) |
There was a problem hiding this comment.
Missing some tests that shows when it's invalid.
There was a problem hiding this comment.
Also when 2,3, more quotes are used. Or when more groups of n quotes are used.
Also when start and end char are not the same (though I mention I don't see the point in another comment)
There was a problem hiding this comment.
when what is invalid ? I don't get it ?
There was a problem hiding this comment.
All the tests are positive. You need to add some to show when it's not valid and Fails() is called. To be sure that it doesn't "match" when it shouldn't.
| /// This method doesn't escape the string, but only validates its content is syntactically correct. | ||
| /// The resulting Span contains the original quotes. | ||
| /// </remarks> | ||
| public bool ReadNonEscapableSequence(char startSequenceChar, char endSequenceChar, out TokenResult result) |
There was a problem hiding this comment.
I don't understand the need of endSequenceChar. If the goal of the PR is to read strings that are not escaped, then the method should be called ReadNonEscapableString and fetch the first char (delimiter) by itself, like the other ReadString methods are doing in this class.
This one looks more generic, but at the same time I am not convinced about the added complexity of being able to set a different start/end delimiter.
There was a problem hiding this comment.
the purpose is also to read sql escapes like [ ] in tsql or `` in mysql, ...
There was a problem hiding this comment.
Cool. Any link to examples of things that are not word and still can be escaped by doubling it?
There was a problem hiding this comment.
Not sure about the[] example. It's more like a Between parser that exists if there is no escaping to check. Unless we can use [ between[] ? If not then I continue to think no arguments are necessary.
There was a problem hiding this comment.
I only found a stackoverflow link https://stackoverflow.com/questions/4200351/what-characters-are-valid-in-an-sql-server-database-name, but I can tell you for sure that you can have almost anything as a character as long as you escape it with []. Here is a doc for mysql: https://dev.mysql.com/doc/refman/8.0/en/identifiers.html
| var start = Cursor.Position; | ||
|
|
||
| // If the next escape if not before the next quote, we can return the string as-is | ||
| Cursor.Advance(nextQuote + 2 - startOffset); |
There was a problem hiding this comment.
It's a nice optimization to call Advance only once, but then you are missing on EOF checks. So a string like 'foo will fail. Which shows some unit tests are missing.
|
If you are actually working on a SQL parser, we need to migrate this one to Parlot. https://github.com/OrchardCMS/OrchardCore/blob/dev/src/OrchardCore.Modules/OrchardCore.Queries/Sql/SqlGrammar.cs |
|
I'm actually working on a usql-like parser, so a mix a sql and c# |
added some more examples for tsql like escapes
Hi Seb,
I was missing the possibility to read string like in SQL where you escape using a double single quote (or as in C# when using the @string notation)
Please let me know if you need more that just the implementation.
PS: sorry for all the other changes, I have automated formatting when saving.