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

Support MarshalText/UnmarshalText for map keys #863

Merged
merged 1 commit into from May 9, 2023
Merged

Support MarshalText/UnmarshalText for map keys #863

merged 1 commit into from May 9, 2023

Conversation

gordon-klotho
Copy link
Contributor

This change enables using types with MarshalText/UnmarshalText as keys. This behaviour matches encoding/json and gopkg.in/yaml.v3 (one of, if not the most popular YAML library).

The only potential breaking change is that maps with unsupported key types only fail if there are elements in the map (see updated tests). This behaviour can be restored by adding the validation before iteration, but given the potential overhead, I didn't want to assume it would be desired.


Benchstat

goos: linux
goarch: amd64
pkg: github.com/pelletier/go-toml/v2/benchmark
cpu: AMD Ryzen 9 5950X 16-Core Processor
                                  │   old.txt    │              new.txt               │
                                  │    sec/op    │    sec/op     vs base              │
UnmarshalDataset/config-2           18.01m ± ∞ ¹   18.06m ± ∞ ¹       ~ (p=0.841 n=5)
UnmarshalDataset/canada-2           70.13m ± ∞ ¹   70.09m ± ∞ ¹       ~ (p=1.000 n=5)
UnmarshalDataset/citm_catalog-2     24.97m ± ∞ ¹   25.43m ± ∞ ¹       ~ (p=0.151 n=5)
UnmarshalDataset/twitter-2          9.714m ± ∞ ¹   9.404m ± ∞ ¹  -3.18% (p=0.008 n=5)
UnmarshalDataset/code-2             82.25m ± ∞ ¹   81.41m ± ∞ ¹       ~ (p=0.151 n=5)
UnmarshalDataset/example-2          137.6µ ± ∞ ¹   139.2µ ± ∞ ¹       ~ (p=0.056 n=5)
Unmarshal/SimpleDocument/struct-2   499.1n ± ∞ ¹   507.9n ± ∞ ¹       ~ (p=0.310 n=5)
Unmarshal/SimpleDocument/map-2      701.8n ± ∞ ¹   693.3n ± ∞ ¹       ~ (p=0.056 n=5)
Unmarshal/ReferenceFile/struct-2    39.68µ ± ∞ ¹   39.95µ ± ∞ ¹       ~ (p=0.310 n=5)
Unmarshal/ReferenceFile/map-2       60.46µ ± ∞ ¹   60.12µ ± ∞ ¹       ~ (p=0.095 n=5)
Unmarshal/HugoFrontMatter-2         10.61µ ± ∞ ¹   10.59µ ± ∞ ¹       ~ (p=0.690 n=5)
Marshal/SimpleDocument/struct-2     361.9n ± ∞ ¹   365.7n ± ∞ ¹  +1.05% (p=0.008 n=5)
Marshal/SimpleDocument/map-2        498.0n ± ∞ ¹   494.0n ± ∞ ¹  -0.80% (p=0.008 n=5)
Marshal/ReferenceFile/struct-2      29.37µ ± ∞ ¹   29.25µ ± ∞ ¹       ~ (p=0.421 n=5)
Marshal/ReferenceFile/map-2         39.05µ ± ∞ ¹   38.43µ ± ∞ ¹  -1.59% (p=0.008 n=5)
Marshal/HugoFrontMatter-2           7.397µ ± ∞ ¹   7.282µ ± ∞ ¹       ~ (p=0.095 n=5)
geomean                             95.42µ         95.19µ        -0.23%
¹ need >= 6 samples for confidence interval at level 0.95

                                  │    old.txt    │               new.txt               │
                                  │      B/s      │      B/s       vs base              │
UnmarshalDataset/config-2           55.54Mi ± ∞ ¹   55.37Mi ± ∞ ¹       ~ (p=0.841 n=5)
UnmarshalDataset/canada-2           29.94Mi ± ∞ ¹   29.95Mi ± ∞ ¹       ~ (p=1.000 n=5)
UnmarshalDataset/citm_catalog-2     21.31Mi ± ∞ ¹   20.92Mi ± ∞ ¹       ~ (p=0.151 n=5)
UnmarshalDataset/twitter-2          43.38Mi ± ∞ ¹   44.81Mi ± ∞ ¹  +3.30% (p=0.008 n=5)
UnmarshalDataset/code-2             31.12Mi ± ∞ ¹   31.44Mi ± ∞ ¹       ~ (p=0.127 n=5)
UnmarshalDataset/example-2          56.15Mi ± ∞ ¹   55.48Mi ± ∞ ¹  -1.19% (p=0.048 n=5)
Unmarshal/SimpleDocument/struct-2   21.02Mi ± ∞ ¹   20.66Mi ± ∞ ¹       ~ (p=0.310 n=5)
Unmarshal/SimpleDocument/map-2      14.94Mi ± ∞ ¹   15.13Mi ± ∞ ¹       ~ (p=0.056 n=5)
Unmarshal/ReferenceFile/struct-2    126.0Mi ± ∞ ¹   125.1Mi ± ∞ ¹       ~ (p=0.310 n=5)
Unmarshal/ReferenceFile/map-2       82.67Mi ± ∞ ¹   83.14Mi ± ∞ ¹       ~ (p=0.095 n=5)
Unmarshal/HugoFrontMatter-2         49.10Mi ± ∞ ¹   49.15Mi ± ∞ ¹       ~ (p=0.690 n=5)
Marshal/SimpleDocument/struct-2     31.62Mi ± ∞ ¹   31.30Mi ± ∞ ¹  -1.03% (p=0.008 n=5)
Marshal/SimpleDocument/map-2        22.98Mi ± ∞ ¹   23.16Mi ± ∞ ¹  +0.79% (p=0.008 n=5)
Marshal/ReferenceFile/struct-2      66.80Mi ± ∞ ¹   67.06Mi ± ∞ ¹       ~ (p=0.397 n=5)
Marshal/ReferenceFile/map-2         48.98Mi ± ∞ ¹   49.77Mi ± ∞ ¹  +1.62% (p=0.008 n=5)
Marshal/HugoFrontMatter-2           67.42Mi ± ∞ ¹   68.49Mi ± ∞ ¹       ~ (p=0.095 n=5)
geomean                             41.21Mi         41.31Mi        +0.24%
¹ need >= 6 samples for confidence interval at level 0.95

                                  │    old.txt    │                new.txt                │
                                  │     B/op      │     B/op       vs base                │
UnmarshalDataset/config-2           5.502Mi ± ∞ ¹   5.502Mi ± ∞ ¹       ~ (p=1.000 n=5)
UnmarshalDataset/canada-2           76.31Mi ± ∞ ¹   76.31Mi ± ∞ ¹       ~ (p=0.524 n=5)
UnmarshalDataset/citm_catalog-2     32.90Mi ± ∞ ¹   32.90Mi ± ∞ ¹       ~ (p=0.548 n=5)
UnmarshalDataset/twitter-2          12.07Mi ± ∞ ¹   12.07Mi ± ∞ ¹       ~ (p=0.690 n=5)
UnmarshalDataset/code-2             20.29Mi ± ∞ ¹   20.29Mi ± ∞ ¹       ~ (p=0.389 n=5)
UnmarshalDataset/example-2          181.2Ki ± ∞ ¹   181.2Ki ± ∞ ¹       ~ (p=0.056 n=5)
Unmarshal/SimpleDocument/struct-2     805.0 ± ∞ ¹     805.0 ± ∞ ¹       ~ (p=1.000 n=5) ²
Unmarshal/SimpleDocument/map-2      1.106Ki ± ∞ ¹   1.106Ki ± ∞ ¹       ~ (p=1.000 n=5) ²
Unmarshal/ReferenceFile/struct-2    19.99Ki ± ∞ ¹   19.99Ki ± ∞ ¹       ~ (p=1.000 n=5) ²
Unmarshal/ReferenceFile/map-2       36.86Ki ± ∞ ¹   36.86Ki ± ∞ ¹       ~ (p=0.278 n=5)
Unmarshal/HugoFrontMatter-2         7.267Ki ± ∞ ¹   7.267Ki ± ∞ ¹       ~ (p=1.000 n=5) ²
Marshal/SimpleDocument/struct-2       240.0 ± ∞ ¹     240.0 ± ∞ ¹       ~ (p=1.000 n=5) ²
Marshal/SimpleDocument/map-2          272.0 ± ∞ ¹     272.0 ± ∞ ¹       ~ (p=1.000 n=5) ²
Marshal/ReferenceFile/struct-2      27.64Ki ± ∞ ¹   27.64Ki ± ∞ ¹       ~ (p=1.000 n=5) ²
Marshal/ReferenceFile/map-2         27.28Ki ± ∞ ¹   27.28Ki ± ∞ ¹       ~ (p=1.000 n=5) ²
Marshal/HugoFrontMatter-2           5.672Ki ± ∞ ¹   5.672Ki ± ∞ ¹       ~ (p=1.000 n=5) ²
geomean                             74.26Ki         74.26Ki        -0.00%
¹ need >= 6 samples for confidence interval at level 0.95
² all samples are equal

                                  │   old.txt    │               new.txt                │
                                  │  allocs/op   │  allocs/op    vs base                │
UnmarshalDataset/config-2           219.2k ± ∞ ¹   219.2k ± ∞ ¹       ~ (p=1.000 n=5)
UnmarshalDataset/canada-2           670.4k ± ∞ ¹   670.4k ± ∞ ¹       ~ (p=1.000 n=5) ²
UnmarshalDataset/citm_catalog-2     178.0k ± ∞ ¹   178.0k ± ∞ ¹       ~ (p=1.000 n=5)
UnmarshalDataset/twitter-2          54.84k ± ∞ ¹   54.84k ± ∞ ¹       ~ (p=0.802 n=5)
UnmarshalDataset/code-2             1.001M ± ∞ ¹   1.001M ± ∞ ¹       ~ (p=1.000 n=5) ²
UnmarshalDataset/example-2          1.306k ± ∞ ¹   1.306k ± ∞ ¹       ~ (p=1.000 n=5) ²
Unmarshal/SimpleDocument/struct-2    9.000 ± ∞ ¹    9.000 ± ∞ ¹       ~ (p=1.000 n=5) ²
Unmarshal/SimpleDocument/map-2       13.00 ± ∞ ¹    13.00 ± ∞ ¹       ~ (p=1.000 n=5) ²
Unmarshal/ReferenceFile/struct-2     160.0 ± ∞ ¹    160.0 ± ∞ ¹       ~ (p=1.000 n=5) ²
Unmarshal/ReferenceFile/map-2        619.0 ± ∞ ¹    619.0 ± ∞ ¹       ~ (p=1.000 n=5) ²
Unmarshal/HugoFrontMatter-2          141.0 ± ∞ ¹    141.0 ± ∞ ¹       ~ (p=1.000 n=5) ²
Marshal/SimpleDocument/struct-2      8.000 ± ∞ ¹    8.000 ± ∞ ¹       ~ (p=1.000 n=5) ²
Marshal/SimpleDocument/map-2         9.000 ± ∞ ¹    9.000 ± ∞ ¹       ~ (p=1.000 n=5) ²
Marshal/ReferenceFile/struct-2       317.0 ± ∞ ¹    317.0 ± ∞ ¹       ~ (p=1.000 n=5) ²
Marshal/ReferenceFile/map-2          429.0 ± ∞ ¹    429.0 ± ∞ ¹       ~ (p=1.000 n=5) ²
Marshal/HugoFrontMatter-2            85.00 ± ∞ ¹    85.00 ± ∞ ¹       ~ (p=1.000 n=5) ²
geomean                             1.060k         1.060k        +0.00%
¹ need >= 6 samples for confidence interval at level 0.95
² all samples are equal

Coverage

Results: v2 97.1% HEAD 97.1%
Delta: 0

Copy link
Owner

@pelletier pelletier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I think the type check not being present is fine. If someone reports an issue about it we'll revisit it. Thank you for taking care of this.

@pelletier pelletier added the feature Issue asking for a new feature in go-toml. label May 9, 2023
@pelletier pelletier changed the title Support text Un/Marshaller for map keys Support MarshalText/UnmarshalText for map keys May 9, 2023
@pelletier pelletier merged commit d34104d into pelletier:v2 May 9, 2023
9 checks passed
@manunio manunio mentioned this pull request May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Issue asking for a new feature in go-toml.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants