Skip to content
This repository has been archived by the owner on Sep 24, 2022. It is now read-only.

Allow empty table keys #373

Merged
merged 1 commit into from Apr 26, 2021
Merged

Conversation

mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Jan 31, 2020

I found this issue by writing a roundtrip property test using quickcheck - master...mgsloan:roundtrip-property-test .

Quoting https://github.com/toml-lang/toml/tree/68076ffc6d9a150d48f1df964551283542ad47bc#user-content-keys

A bare key must be non-empty, but an empty quoted key is allowed (though discouraged).

See also the discussion in toml-lang/toml#432

@mgsloan mgsloan force-pushed the allow-empty-table-keys branch 2 times, most recently from 52ce649 to e5bc5bc Compare January 31, 2020 07:16
@mgsloan
Copy link
Contributor Author

mgsloan commented Jan 31, 2020

Looks like empty keys were quite intentionally disallowed. I can see this being diagnostically nice for many applications, but toml allows it, so I think it's better to match the spec.

Quoting https://github.com/toml-lang/toml/tree/68076ffc6d9a150d48f1df964551283542ad47bc#user-content-keys

> A bare key must be non-empty, but an empty quoted key is allowed (though discouraged).

See also the discussion in toml-lang/toml#432
@mgsloan
Copy link
Contributor Author

mgsloan commented Feb 1, 2020

Note that the reason this was found by a property test is that the serializer will happily output invalid toml when serializing a table with an empty key.

@joelthelion
Copy link

Could this fix be merged? The current behavior clearly goes against the spec.

Copy link
Collaborator

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thank for the PR! It is a little strange that they decided to allow empty keys, but since it is explicitly noted in the spec, I agree it should probably be allowed.

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

Successfully merging this pull request may close these issues.

None yet

3 participants