pelletier / go-toml

Go library for the TOML file format
https://github.com/pelletier/go-toml
Other
1.69k stars 206 forks source link

Questionable behavior of 'omitempty' tag #786

Closed Felixoid closed 2 years ago

Felixoid commented 2 years ago

Describe the bug Hello. I see the current behavior of omitempty flag as a bit confusing because of a few factors:

  1. It creates empty lines at the place records should be
  2. For the slices of structures it's inherited and changes existing tags of embedded structures

To Reproduce

package main

import (
    "bytes"
    "fmt"

    "github.com/pelletier/go-toml/v2"
)

type General struct {
    From      string `toml:"from,omitempty" json:"from,omitempty" comment:"from in graphite-web format, the local TZ is used"`
    Randomize bool   `toml:"randomize" json:"randomize" comment:"randomize starting time with [0,step)"`
}

type Custom struct {
    Name string `toml:"name" json:"name,omitempty" comment:"names for generator, braces are expanded like in shell"`
    Type string `toml:"type" json:"type,omitempty" comment:"type of generator"`
    General
}
type Config struct {
    General
    Custom []Custom `toml:"custom,omitempty" json:"custom,omitempty" comment:"generators with custom parameters can be specified separately"`
}

func main() {
    buf := new(bytes.Buffer)
    config := &Config{General: General{From: "-2d", Randomize: true}}
    config.Custom = []Custom{{Name: "omit", General: General{Randomize: false}}}
    config.Custom = append(config.Custom, Custom{Name: "present", General: General{From: "-2d", Randomize: true}})
    encoder := toml.NewEncoder(buf)
    encoder.Encode(config)
    fmt.Print(buf.String())
}

it produces (includes empty line on the end)

# from in graphite-web format, the local TZ is used
from = '-2d'
# randomize starting time with [0,step)
randomize = true
# generators with custom parameters can be specified separately
[[custom]]
# names for generator, braces are expanded like in shell
name = 'omit'

[[custom]]
# names for generator, braces are expanded like in shell
name = 'present'

# from in graphite-web format, the local TZ is used
from = '-2d'
# randomize starting time with [0,step)
randomize = true

Expected behavior I expect another result:

# from in graphite-web format, the local TZ is used
from = '-2d'
# randomize starting time with [0,step)
randomize = true
# generators with custom parameters can be specified separately
[[custom]]
# names for generator, braces are expanded like in shell
name = 'omit'
# randomize starting time with [0,step)
randomize = false
[[custom]]
# names for generator, braces are expanded like in shell
name = 'present'
# from in graphite-web format, the local TZ is used
from = '-2d'
# randomize starting time with [0,step)
randomize = true

Versions

Additional context Add any other context about the problem here that you think may help to diagnose.

Felixoid commented 2 years ago

Not sure if it's worth creating a new issue, so I'll ask here. I can't set alignment for toml lists in the encoder. SetIndentSymbol doesn't affect it now. Is it expected behavior?

pelletier commented 2 years ago

Thanks for the bug report!

It creates empty lines at the place records should be

You're right, those extra new lines shouldn't be emitted.

For the slices of structures it's inherited and changes existing tags of embedded structures

I am not sure I understand the problem. Can you share an example?

SetIndentSymbol doesn't affect it now. Is it expected behavior?

Do you have some code to reproduce the issue?

Felixoid commented 2 years ago

I am not sure I understand the problem. Can you share an example?

Yes, see the first [custom] section

# given
[[custom]]
# names for generator, braces are expanded like in shell
name = 'omit'

# expected
[[custom]]
# names for generator, braces are expanded like in shell
name = 'omit'
# randomize starting time with [0,step)
randomize = false

Randomize bool `toml:"randomize" doesn't contain omitempty, but it's omitted in the output.

Do you have some code to reproduce the issue?

yes, if I use encoder := toml.NewEncoder(buf).SetIndentSymbol(" ") it does nothing with the output. I'd like to have [[custom]] content aligned.

[[custom]]
  # names for generator, braces are expanded like in shell
  name = 'omit'
  # randomize starting time with [0,step)
  randomize = false
stingalleman commented 2 years ago

Hi, also having this issue. It looks like go-toml is creating new lines when omit empty is defined, instead of discarding the lines.

Also, I expect objects like in the example below to be omitted when all the keys in the object are omitted.

type Dependencies struct {
    Dependencies         []string `toml:"dependencies,multiline,omitempty"`
    BuildDependencies    []string `toml:"buildDependencies,multiline,omitempty"`
    OptionalDependencies []string `toml:"optionalDependencies,multiline,omitempty"`
}

type Test struct {
    Dependencies Dependencies `toml:"dependencies,omitempty"
}

Result: (notice empty lines)

[dependencies]

Expected result would be an empty file.

pelletier commented 2 years ago

@Felixoid for the indentation issue, you also need to .SetIndentTables(true) for the encoder to indent tables (and use whatever indentation string was provided to SetIndentSymbol).

I started working on those extra newlines and omitempty issues. It's going to take a bit because most tests were using equalStringsIgnoreNewlines, which effectively hid those issues :(

pelletier commented 2 years ago

@Felixoid @stingalleman Pull requests #803 and #798 have been merged. Unless I missed something those two should fix both the extra new lines and the incorrect propagation of omitempty. Let me know if it works!

Felixoid commented 1 year ago

Hello, I didn't come right after the fix. I've had a corona, and then just everything went south ¯\_(ツ)_/¯

The output now is correct in regard to the omitempty, but SetIndentTables(true) doesn't trigger the desired behavior.

Should I create a separate issue, or could we discuss it here?

pelletier commented 1 year ago

@Felixoid no worries! Thank you for letting me know. I went ahead and filed a new issue https://github.com/pelletier/go-toml/issues/888