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

Line ending backslash in """ causes later line breaks to also be removed #372

Closed
JoelColledge opened this issue Nov 8, 2022 · 8 comments · Fixed by #391
Closed

Line ending backslash in """ causes later line breaks to also be removed #372

JoelColledge opened this issue Nov 8, 2022 · 8 comments · Fixed by #391
Labels

Comments

@JoelColledge
Copy link

Hi, I'm having trouble with breaking long lines in """ delimited strings using a backslash. This is called a "line ending backslash" in the toml spec.

Reproducer:

package main

import (
	"fmt"
	"os"

	"github.com/BurntSushi/toml"
)

const in = `f = """
a\
b
c
"""`

func main() {
	var out map[string]string
	if _, err := toml.Decode(in, &out); err != nil {
		fmt.Fprintf(os.Stderr, "error: %v\n", err)
		os.Exit(1)
	}

	fmt.Println(out["f"])
}

Expected output, which is generated with BurntSushi/toml v0.3.1:

ab
c

Output with BurntSushi/toml v1.2.1:

abc

Any chance of fixing this? Thanks!

@arp242
Copy link
Collaborator

arp242 commented Nov 8, 2022

This probably broke quite a few releases ago, as this hasn't been touched since last year; I'm guessing with 35432de.

I should probably rewrite that entire logic; I'm having trouble following it now.

@cespare
Copy link
Collaborator

cespare commented May 23, 2023

Yikes. I sent #391.

@arp242 would you mind giving that a review and then cutting a 1.2.2 release? Or I'm happy to do it, though I figure you would have a better idea of what all to mention in the release notes.

@arp242
Copy link
Collaborator

arp242 commented May 23, 2023

would you mind giving that a review and then cutting a 1.2.2 release? Or I'm happy to do it, though I figure you would have a better idea of what all to mention in the release notes.

Let me take a look at #371 before the release; then every (known) bug should be fixed.

@cespare
Copy link
Collaborator

cespare commented May 23, 2023

@arp242 I took at stab at that in #392 to try to speed things along.

However, I would say that this is a much more serious issue since (I'd guess) orders of magnitude more code depends on decoding than encoding. This bug caused a configuration issue at my work which is how it came to my attention.

@arp242
Copy link
Collaborator

arp242 commented May 23, 2023

I would say that this is a much more serious issue since (I'd guess) orders of magnitude more code depends on decoding than encoding.

I agree; I would (should) have fixed it much earlier; I just haven't been able to spend as much time on things in the last few months as I'd like due to some personal issues; I've been picking up things again since last week, but there's a bit of a backlog (so many emails...). I actually did create a quick fix yesterday, but your PR pre-empted that (and it's nicer anyway).

However, it seems to have been broken since June 2021, so it's less often used than I would expect, and in that context a few more days probably won't matter too much, I think? It avoids having to push out a new bugfix release just for that.

@cespare
Copy link
Collaborator

cespare commented May 23, 2023

However, it seems to have been broken since June 2021, so it's less often used than I would expect,

One thing that makes that a bit tricky to evaluate is that people might not be pulling in the latest version of this package very quickly in well-established code bases with a lot of TOML given how MVS works. At my work we only pulled in this bug about five months ago.

and in that context a few more days probably won't matter too much, I think? It avoids having to push out a new bugfix release just for that.

Sure, a few days is fine by me. And as I said I'm also happy to run the release if that would help.

@arp242
Copy link
Collaborator

arp242 commented May 30, 2023

I tagged v1.3.0.

@cespare
Copy link
Collaborator

cespare commented May 30, 2023

@arp242 thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants