Add test case for multiline string #610

jon-ht · 2020-05-15T16:43:27Z

In YamlSourceManipulator.php line 793:
                                                                     
  Cannot find the original value "Symfony 5 project                  
                                                                     
  Some comments with markdown like [/api/doc/users](/api/doc/users)  
  or `/api/doc/{area}`                                               
  "

weaverryan · 2020-05-15T16:58:41Z

This test case doesn't actually expose the error you're talking about. It does expose a formatting bug:

 'nelmio_api_doc:
     documentation:
         info:
-            description: |
-                Multiline string'
+            description: "Multiline string\n"'

But it does not show the error you're seeing.

jon-ht · 2020-05-15T17:31:31Z

It's weird, I could reproduce the error from CI but not always. It seems to be related to file ending (CRLF)

Sometime having one line of description give the expected error, sometimes not, can't understand why or what is happening

weaverryan

Hey @jon-ht!

You went the extra mile and fixed the bug too! Congrats on navigating my horribly confusing code in YamlSourceManipulator :)

I've left some minor comments. It's mostly that we need to do our best to keep this complex class as understandable as possible.

Thanks!

weaverryan · 2020-05-23T18:18:10Z

tests/Util/YamlSourceManipulatorTest.php


-            $data = Yaml::parse($source);
+            // Multiline string ends with an \n
+            $data = Yaml::parse(trim($source, "\n"));


We actually trim this below already. So it looks like we just need to trim it earlier. I'm thinking the whole inside of the foreach would be this:

list($source, $changeCode, $expected) = explode('===', $file->getContents()); $source = rtrim($source, "\n"); $expected = ltrim($expected, "\n"); $data = Yaml::parse($source); eval($changeCode); yield $file->getFilename() => [ 'source' => $source, 'newData' => $data, 'expectedSource' => $expected, ];

Good catch, I missed this !

weaverryan · 2020-05-23T18:18:19Z

tests/Util/yaml_fixtures/multiline_string.test

+            description: |
+                Multiline string
+                Second line
+                Third line


beautiful test cases

weaverryan · 2020-05-23T18:21:30Z

src/Util/YamlSourceManipulator.php

+                $patternValue = $quotedValue;
+
+                // Iterates until we find a new line char or we reach end of file
+                if ($this->findPositionOfMultilineCharInLine($offset)) {


Suggested change

if ($this->findPositionOfMultilineCharInLine($offset)) {

if ($this->findPositionOfMultilineCharInLine($offset) !== null) {

That will help clarify that we're looking for IF there is a multiline char here, we're not (in this case) actually caring about the exact position returned.

weaverryan · 2020-05-23T18:25:38Z

src/Util/YamlSourceManipulator.php

+                && rtrim($currentVal, "\n") !== $currentVal
+                && rtrim($newVal, "\n") === $newVal
+            ) {
+                $newVal .= "\n";


Could you add a $this->log() line here that explains what you're doing? There's a little-known feature where, in YamlSourceManipulatorTest.php, you can uncomment a setLogger() line to see all these logs entries. And it helps document what each part does, since it's SO complex. For instance, in this case, I'm not entirely sure what edge-case this is fixing.

Well, it seems not required. I think I went crazy and wanted to add/remove \n everywhere 😄.
I will remove completely

weaverryan · 2020-05-23T18:30:09Z

src/Util/YamlSourceManipulator.php

-        $newYamlValue = $this->convertToYaml($value);
-        if (!\is_array($originalVal) && \is_array($value)) {
+        $isMultilineValue = null !== $this->findPositionOfMultilineCharInLine($this->currentPosition);
+        $originalValEndsWithNewLine = $isMultilineValue ? rtrim($originalVal, "\n") !== $originalVal : false;


I think the rtrim($originalVal, "\n") !== $originalVal is confusing me. What about this instead?

str_ends_with($originalValue, "\n")

That's a PHP 8 function, but if you require "symfony/polyfill-php80": "^1.15", then we can use it.

Are you OK to require symfony/polyfill-php80 then ? I'll made apprioriate changes if so ~~(there are some other lines like https://github.com/symfony/maker-bundle/pull/610/files#diff-a07d297f64d0bf4a1e9c902b787ecf20R219-R223)~~

weaverryan · 2020-05-23T18:32:48Z

src/Util/YamlSourceManipulator.php

+
+        // In case of multiline, $value is converted as plain string like "Foo\nBar"
+        // We need to keep it "as is"
+        $newYamlValue = $isMultilineValue ? rtrim($value, "\n") : $this->convertToYaml($value);


Why do we need to rtrim() here? I can see that if I remove it the tests fail, but I can't figure out why it's needed.

One thing I noticed is, in this test case multiline_string_before_property.test, $value ends with an \n

This will cause indentMultilineYamlArray to add an extra line that's not wanted.

Ah, I understand. I think the implementation is what's confusing me. All multiline arrays must end in a line break, by their very nature. If anything, in this spot, (just thinking about the logical flow of the code) I would expect you to be guaranteeing that the $newYamlValue does end in \n, instead of stripping it. After all, if we made sure that $newYamlValue ended in 1 line break... then that should work perfectly with YAML, which also wants the line break :). I'm wondering if we're trimming and adding \n in so many places that we're doing and undoing extra work.

There is the edge-case of not needing the "\n" if the multiline string is at the end of the file. That should be handled (somewhere) with an if statement specifically for it. Something like:

// maybe down before the `$newContents = substr(...)` line if (false === $this->findNextLineBreak($newPosition) && $isMultilineValue) { $newYamlValue = trim($newYamlValue, "\n"); }

I won't lie to you, I also think there is a lot of trimming to handle multiline string...

Actually, it seems that re-adding original \n (after $newYamlValue = "\n".$this->indentMultilineYamlArray($newYamlValue);) breaks test case because, as mentionned above, YamlDumper will add an extra line

Maybe this bug is related to Yaml component and not YamlSourceManipulator

weaverryan · 2020-05-24T11:02:55Z

Thanks for taking this on - I knew that I didn't cover this case when I built YamlSourceManipulator, and I wasn't looking forward to trying to do it - cause it's hard ;).

jon-ht · 2020-05-24T11:51:40Z

I was able to remove some useless trimming after a second pass, maybe this is less confusing now.

weaverryan · 2020-05-29T14:44:51Z

Thanks for solving this SUPER ugly shortcoming @jon-ht!

Add test case for multiline string

ee7e823

jon-ht mentioned this pull request May 18, 2020

YamlSourceManipulator cannot find original value for multline string #608

Closed

weaverryan mentioned this pull request May 22, 2020

YamlSourceManipulator: insert new element into indexed array #618

Open

weaverryan reviewed May 23, 2020

View reviewed changes

remove unnecessary \n adding/removing

661acb0

weaverryan merged commit b5149e9 into symfony:master May 29, 2020

	if ($this->findPositionOfMultilineCharInLine($offset)) {
	if ($this->findPositionOfMultilineCharInLine($offset) !== null) {

Uh oh!

Add test case for multiline string #610

Add test case for multiline string #610

Uh oh!

Conversation

jon-ht commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weaverryan commented May 15, 2020

Uh oh!

jon-ht commented May 15, 2020

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-ht May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-ht May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented May 24, 2020

Uh oh!

jon-ht commented May 24, 2020

Uh oh!

weaverryan commented May 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jon-ht commented May 15, 2020 •

edited

Loading

jon-ht May 23, 2020 •

edited

Loading

jon-ht May 23, 2020 •

edited

Loading