Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

command/format: multi-line rendering for unchanged strings #23305

Merged
merged 1 commit into from
Nov 7, 2019

Conversation

apparentlymart
Copy link
Contributor

We have a special treatment for multi-line strings that are being updated in-place where we show them across multiple lines in the plan output, but we didn't use that same treatment for rendering multi-line strings in isolation such as when they are being added for the first time.

Here we detect when we're rendering a multi-line string in a no-change situation and render it using the diff renderer instead, using the same value for old and new and thus producing a multi-line result without any diff markers at all.

This improves consistency between the change and no-change cases, and makes multi-line strings (such as YAML in block mode) readable in all cases.

Before:

  # null_resource.foo will be created
  + resource "null_resource" "foo" {
      + id       = (known after apply)
      + triggers = {
          + "a" = "foo\nbar\nbaz\n"
        }
    }

After:

  # null_resource.foo will be created
  + resource "null_resource" "foo" {
      + id       = (known after apply)
      + triggers = {
          + "a" = <<~EOT
                foo
                bar
                baz
            EOT
        }
    }

This fixes #21817 as closely as we can within our existing diff heuristics. Specifically, the new treatment of the list-of-multiline-strings case is improved but still not ideal because the list diffing algorithm only ever produces additions and removals and never in-place updates, so a list like the values in that issue's example would use the above multi-line rendering for both the old and new values but would not interleave them into a single line-oriented diff because it doesn't understand that the new element zero is intended as an in-place edit to the old element zero, rather than a replacement. Maybe we'll improve on that further someday, but for now just showing the lines of the string in full should hopefully be a reasonable compromise.

@apparentlymart apparentlymart requested a review from a team November 7, 2019 00:06
@apparentlymart apparentlymart self-assigned this Nov 7, 2019
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Nov 7, 2019
We have a special treatment for multi-line strings that are being updated
in-place where we show them across multiple lines in the plan output, but
we didn't use that same treatment for rendering multi-line strings in
isolation such as when they are being added for the first time.

Here we detect when we're rendering a multi-line string in a no-change
situation and render it using the diff renderer instead, using the same
value for old and new and thus producing a multi-line result without any
diff markers at all.

This improves consistency between the change and no-change cases, and
makes multi-line strings (such as YAML in block mode) readable in all
cases.
@apparentlymart apparentlymart merged commit 7db2825 into master Nov 7, 2019
@apparentlymart apparentlymart deleted the f-diff-multiline-string branch November 7, 2019 23:25
@ghost
Copy link

ghost commented Mar 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cli enhancement sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-line diffs are not displayed properly sometimes
2 participants