pelletier / go-toml

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

Bring back toml.Unmarshaler #873

Open pelletier opened 1 year ago

pelletier commented 1 year ago

Creating a main issue to track this work, as it has been asked multiple times, is probably hindering the adoption of go-toml v2.

The main difficulty of building this feature is modifying the unmarshaler in a way that allows it to retrieve actual bytes from the parser for the current target element without impacting performance.

The good thing is that solving this problem will also make implementingtoml.RawMessage pretty easy (https://github.com/pelletier/go-toml/issues/796)!

In my opinion, the work should probably be carried out in this order:

  1. Add the ability to retrieve the offset in the TOML document where a node's data starts. The recent merge of https://github.com/pelletier/go-toml/pull/860 introducing Position should help with that.
  2. Make the Decoder detect that its current target implements toml.Unmarshaler, then keep calling the parser until it finishes its current structure.
  3. Record the first node's start offset and the last node's end offset to retrieve the raw data from the input document.
  4. Feed the raw data to the UnmarshalTOML method.

Part 2 is probably the most difficult to pull off; the unmarshaling and parsing code is somewhat intertwined – though thanks to the intermediate partial AST design, separating both should be doable. The hard part is keeping the same performance level when decoding non-Unmarshaler targets and trying not to fully duplicate the Decoder (otherwise, it's twice the maintenance work :)).

Attention should also be given to detecting that the type implements toml.Unmarshaler. There's likely going to be a performance penalty there until some parser generation is implemented (https://github.com/pelletier/go-toml/pull/669, https://github.com/pelletier/go-toml/pull/758), but we should aim to minimize the impact of that check.

rszyma commented 3 months ago

940 added only partial support, this shouldnt be close probably

pelletier commented 3 months ago

Thanks for catching it, didn't mean to close this issue!