Multiline string implementation #1542

anthony-c-martin · 2021-02-12T19:21:21Z

Contributing a Pull Request

If you haven't already, read the full contribution guide. The guide may have changed since the last time you read it, so please double-check. Once you are done and ready to submit your PR, run through the relevant checklist below.

Contributing a feature

I have opened a new issue for the proposal, or commented on an existing one, and ensured that the bicep maintainers are good with the design of the feature being implemented
I have included "Fixes #{issue_number}" in the PR description, so GitHub can link to the issue and close it when the PR is merged
I have appropriate test coverage of my new feature

Fixes #416

codecov-io · 2021-02-12T22:06:29Z

Codecov Report

Merging #1542 (4fe963d) into main (c8326cb) will decrease coverage by 0.00%.
The diff coverage is 94.59%.

@@            Coverage Diff             @@
##             main    #1542      +/-   ##
==========================================
- Coverage   94.92%   94.92%   -0.01%     
==========================================
  Files         356      356              
  Lines       19258    19332      +74     
  Branches       14       14              
==========================================
+ Hits        18281    18351      +70     
- Misses        977      981       +4

Flag	Coverage Δ
dotnet	`95.40% <94.59%> (-0.01%)`	⬇️
typescript	`27.20% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/Bicep.LangServer/SemanticTokenVisitor.cs	`0.00% <ø> (ø)`
src/Bicep.Core/Parsing/Parser.cs	`97.90% <83.33%> (-0.16%)`	⬇️
src/Bicep.Core/Parsing/Lexer.cs	`97.72% <90.00%> (-0.73%)`	⬇️
...Core.IntegrationTests/Emit/TemplateEmitterTests.cs	`100.00% <100.00%> (ø)`
src/Bicep.Core.UnitTests/Parsing/LexerTests.cs	`100.00% <100.00%> (ø)`
src/Bicep.Core.UnitTests/Parsing/ParserTests.cs	`100.00% <100.00%> (ø)`
src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs	`100.00% <100.00%> (ø)`

anthony-c-martin · 2021-02-13T07:19:22Z

One thing that is awkward:

If I type ' to start a multiline string, VSCode inserts a terminating ' so I have ''. If I think type another ' (to make it 3 quotes), VSCode does the same thing again, so I end up with 4.

To get VSCode to give me 3 quotes I have to enter the following:

', ', ', ', backspace

If I try ', ', ', backspace, I end up with 2 quotes.

I assume this is related to the config in src/vscode-bicep/language-configuration.json, I'm just not sure how to sort it out.

alex-frankel · 2021-02-13T17:32:29Z

Noticed this as well - maybe it's easiest to switch to a different character like backtick? Would be a good question for Anders

majastrz · 2021-02-16T00:15:14Z

What happens if you just add the new type of string tokens to language-configuration.json? Wondering if VS code has some logic to handle this situation...

anthony-c-martin · 2021-02-16T15:15:02Z

What happens if you just add the new type of string tokens to language-configuration.json? Wondering if VS code has some logic to handle this situation...

Turns out I should have just tried this out :) I've pushed the changes for this.

Now if I type ''', VSCode inserts an additional ''', with the cursor (|) as such '''|'''. It's still slightly awkward because '''''' is interpreted as an opening sequence for a multiline string with 6 consecutive quotes, so this shows a syntax error until the user inserts a character.

alex-frankel · 2021-02-16T16:27:29Z

We might have control over what is inserted on a completion like that. I think you can add a new line and potentially move the second ''' to position zero since that white space matters.

shenglol · 2021-02-18T04:50:12Z

@anthony-c-martin - this might be a rare case, but is it possible to have a multiline string end with '? l could not figure out a way to do that:

anthony-c-martin · 2021-02-18T15:59:46Z

@anthony-c-martin - this might be a rare case, but is it possible to have a multiline string end with '? l could not figure out a way to do that:

No - this would be blocked by this approach since the lexer is looking for exactly 3 ' chars to terminate the string, and will keep scanning if it sees an additional '.

If we were to relax this, you would always need to pick an opening sequence of ' n chars with n > the longest sequence of ' chars in the string you're including - e.g. this would not be possible:

var myString = '''
abc''''''''def
'''

and would instead have to be written as:

var myString = '''''''''
abc''''''''def
'''''''''

I went with this approach to give more freedom to pick the opening sequence, and we hypothesized that the majority of use-cases would not be sensitive to whether or not the string is terminated with a newline.

shenglol · 2021-02-18T18:44:18Z

@anthony-c-martin - this might be a rare case, but is it possible to have a multiline string end with '? l could not figure out a way to do that:

No - this would be blocked by this approach since the lexer is looking for exactly 3 ' chars to terminate the string, and will keep scanning if it sees an additional '.

If we were to relax this, you would always need to pick an opening sequence of ' n chars with n > the longest sequence of ' chars in the string you're including - e.g. this would not be possible:
var myString = '''
abc''''''''def
'''
and would instead have to be written as:
var myString = '''''''''
abc''''''''def
'''''''''
I went with this approach to give more freedom to pick the opening sequence, and we hypothesized that the majority of use-cases would not be sensitive to whether or not the string is terminated with a newline.

Gotcha. This makes sense. Thanks for the explanation.

docs/examples/101/function-http-trigger/main.json

docs/spec/bicep.md

majastrz · 2021-02-18T21:29:22Z

We might have control over what is inserted on a completion like that. I think you can add a new line and potentially move the second ''' to position zero since that white space matters.

It's unfortunately not a completion but a different VS code feature enabled via config. So the snippet solution might now work unless we specifically included that completion everywhere.

majastrz · 2021-02-18T21:31:28Z

Question on the issue @shenglol noticed above. Why can't we have the lexer scan closing quotes and subtract the expected number of closing quotes from the number actually specified? Those that remain could then be included in the string value, no?

anthony-c-martin · 2021-02-18T21:36:00Z

Question on the issue @shenglol noticed above. Why can't we have the lexer scan closing quotes and subtract the expected number of closing quotes from the number actually specified? Those that remain could then be included in the string value, no?

Take a look at my response. That's definitely a valid option, it just puts more of a constraint on the choice of opening quotes.

majastrz · 2021-02-19T20:54:28Z

docs/grammar.md

+stringLeftPiece -> "'" STRINGCHAR* "${"
+stringMiddlePiece -> "}" STRINGCHAR* "${"
+stringRightPiece -> "}" STRINGCHAR* "'"
+stringComplete -> "'" STRINGCHAR* "'"


STRINGCHAR [](start = 22, length = 10)

Not a new issue and definitely non-blocking for this PR, but I just realized we haven't defined what STRINGCHAR is or even IDENTIFIER.

Yes :) I decided to leave it along with this PR

majastrz · 2021-02-19T21:03:16Z

src/Bicep.Core/Parsing/Lexer.cs

+            var startOffset = terminatingQuoteCount;
+
+            // we strip a leading \r\n or \n
+            if (tokenText[startOffset] == '\r')


tokenText [](start = 16, length = 9)

This matches \r\n | \n, but I believe in the lexer elsewhere we match [\r|\n]+ elsewhere. It's subtly inconsistent, but not an issue for CRLF or LF files. I think it's fine for now, but I think we need to centralize our newline handling in the future. To finish unicode support post-0.3, we will also need to support line break and paragraph break chars.

majastrz · 2021-02-19T21:05:25Z

src/Bicep.Core/Parsing/Lexer.cs

@@ -409,6 +450,45 @@ private void ScanNewLine()
            }
        }

+        private TokenType ScanMultilineString()
+        {
+            const int terminatingQuoteCount = 3;


terminatingQuoteCount [](start = 22, length = 21)

This constant should be at the class level and shared between the two functions.

majastrz · 2021-02-19T21:11:43Z

src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs

+            public ErrorDiagnostic UnterminatedMultilineString() => new(
+                TextSpan,
+                "BCP140",
+                $"The multi-line string at this location is not terminated. Terminate it with \"'''\".");


The multi-line string at this location is not terminated. [](start = 18, length = 57)

I don't see a test that produces this error. Can you add a unit or integration test to cover it?

Never mind - found it.

In reply to: 579478035 [](ancestors = 579478035)

Although could you add an integration test so it's more obvious where the error span lands?

In reply to: 579478358 [](ancestors = 579478358,579478035)

majastrz

Signed off with a couple of comments.

anthony-c-martin force-pushed the antmarti/multilines branch from 588991a to 9e3984b Compare February 12, 2021 21:58

anthony-c-martin force-pushed the antmarti/multilines branch from 9e3984b to 9ba896f Compare February 13, 2021 03:44

Multiline string implementation

32499e7

anthony-c-martin force-pushed the antmarti/multilines branch from 9ba896f to 32499e7 Compare February 13, 2021 07:10

anthony-c-martin changed the title ~~[WIP] Multiline strings~~ Multiline string implementation Feb 13, 2021

anthony-c-martin marked this pull request as ready for review February 13, 2021 07:12

Add multiline string chars to language configuration

69e8b00

Merge remote-tracking branch 'origin/main' into antmarti/multilines

c55bf9d

shenglol approved these changes Feb 18, 2021

View reviewed changes

majastrz reviewed Feb 18, 2021

View reviewed changes

docs/examples/101/function-http-trigger/main.json Show resolved Hide resolved

majastrz reviewed Feb 18, 2021

View reviewed changes

docs/spec/bicep.md Outdated Show resolved Hide resolved

majastrz reviewed Feb 18, 2021

View reviewed changes

docs/spec/bicep.md Show resolved Hide resolved

anthony-c-martin added 2 commits February 18, 2021 14:32

Merge remote-tracking branch 'origin/main' into antmarti/multilines

3f0a1b7

Add docs clarity w.r.t the opening and closing multiline sequence

a9156bb

Update spec and implementation to simplify handling # of quotes

4fe963d

anthony-c-martin force-pushed the antmarti/multilines branch from 9d6798d to 4fe963d Compare February 19, 2021 14:51

majastrz reviewed Feb 19, 2021

View reviewed changes

majastrz approved these changes Feb 19, 2021

View reviewed changes

Address PR comments

a6e77b1

anthony-c-martin enabled auto-merge (squash) February 19, 2021 21:24

anthony-c-martin merged commit c80f38f into main Feb 19, 2021

anthony-c-martin deleted the antmarti/multilines branch February 19, 2021 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiline string implementation #1542

Multiline string implementation #1542

anthony-c-martin commented Feb 12, 2021 •

edited

Loading

codecov-io commented Feb 12, 2021 •

edited

Loading

anthony-c-martin commented Feb 13, 2021

alex-frankel commented Feb 13, 2021

majastrz commented Feb 16, 2021

anthony-c-martin commented Feb 16, 2021

alex-frankel commented Feb 16, 2021

shenglol commented Feb 18, 2021

anthony-c-martin commented Feb 18, 2021

shenglol commented Feb 18, 2021

majastrz commented Feb 18, 2021

majastrz commented Feb 18, 2021 •

edited

Loading

anthony-c-martin commented Feb 18, 2021

majastrz Feb 19, 2021

anthony-c-martin Feb 19, 2021

majastrz Feb 19, 2021

majastrz Feb 19, 2021

majastrz Feb 19, 2021

majastrz Feb 19, 2021

majastrz Feb 19, 2021

majastrz left a comment

Multiline string implementation #1542

Multiline string implementation #1542

Conversation

anthony-c-martin commented Feb 12, 2021 • edited Loading

Contributing a Pull Request

Contributing a feature

codecov-io commented Feb 12, 2021 • edited Loading

Codecov Report

anthony-c-martin commented Feb 13, 2021

alex-frankel commented Feb 13, 2021

majastrz commented Feb 16, 2021

anthony-c-martin commented Feb 16, 2021

alex-frankel commented Feb 16, 2021

shenglol commented Feb 18, 2021

anthony-c-martin commented Feb 18, 2021

shenglol commented Feb 18, 2021

majastrz commented Feb 18, 2021

majastrz commented Feb 18, 2021 • edited Loading

anthony-c-martin commented Feb 18, 2021

majastrz Feb 19, 2021

Choose a reason for hiding this comment

anthony-c-martin Feb 19, 2021

Choose a reason for hiding this comment

majastrz Feb 19, 2021

Choose a reason for hiding this comment

majastrz Feb 19, 2021

Choose a reason for hiding this comment

majastrz Feb 19, 2021

Choose a reason for hiding this comment

majastrz Feb 19, 2021

Choose a reason for hiding this comment

majastrz Feb 19, 2021

Choose a reason for hiding this comment

majastrz left a comment

Choose a reason for hiding this comment

anthony-c-martin commented Feb 12, 2021 •

edited

Loading

codecov-io commented Feb 12, 2021 •

edited

Loading

majastrz commented Feb 18, 2021 •

edited

Loading