-
Notifications
You must be signed in to change notification settings - Fork 858
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
Allow newlines and trailing comma in inline tables #904
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not delete the existing example. Looks good otherwise!
Great work, @arp242. Thanks for stepping up! Would it be worthwhile to mention, within the context of the spec as a whole, that the inline table's purpose is to make nesting simpler when needed? We threw out a chunk of formal editorializing, but I'd think just adding this one bit keeps inline tables in their proper perspective. |
I restored the previous example and added a new one; I spent a good few minutes staring at my screen trying to think of an example of nested data, and this was the best I could come up with right now 😅 I also added a mention of "nested" in the description. |
There was a problem hiding this 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!
@pradyunsg Care to take a look and merge, if you agree?
toml.md
Outdated
@@ -797,7 +797,9 @@ Inline Table | |||
------------ | |||
|
|||
Inline tables provide a more compact syntax for expressing tables. They are | |||
especially useful for grouped data that can otherwise quickly become verbose. | |||
especially useful for grouped data that can otherwise quickly become verbose or | |||
(deeply) nested data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty good. Could you make one subtle change to the last line? If you remove the word "data", it would then describe the types of grouped data that inline tables can help to manage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked the wording now to:
They are especially useful for grouped (deeply) nested data that can otherwise quickly
become verbose.
which I felt was a better phrasing, and should address the intent of your comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must admit that I liked @eksortso 's wording better. "grouped (deeply) nested data" just feels like too many adjectives in a row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I preferred it exactly because it put all adjectives in one place, instead of "amending" it later.
I don't strongly care either way though; I'll leave it as is for now but I'll happily change it if eksortso or other people agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably, allowing newlines makes inline tables look less crowded, even if they aren't nested. So even with all the adjectives, it gets across the point that this change is about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to ignore the "grouped" part. That's also a feature, because the inline syntax puts subtables in the middle of a table block without breaking the block apart. Not being able to do so was an issue that visitors have expressed in the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, you got this!
@pradyunsg @mojombo Please review this. It's ready to merge if it's acceptable to you. |
@pradyunsg @mojombo Please respond. It's been over two months. If you would merge this PR, it would resolve issue #516, and it would address the deep-nesting issue that's been boiling for years. At the very least, please say something. |
I’ve been swamped by a few IRL things, so I’ve not been looking to closely at this repository. I’ll make some time this week and catch up. 😅 |
Thanks, @pradyunsg. One way or another, this PR will be a big hit with us. And speaking as a guy with IRL problems of his own, I will tell you now, I am grateful to you. |
Thanks for being understanding folks! :) |
We are very understanding ... but only as long as you don't run away again! (insert threatening face) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with A couple of language nit picks.
Fair enough. :P |
Thanks for the PR @arp242 and for the discussions here everyone! :) |
That's great to see! Two questions, though:
|
One thing that we need to do is write test cases in toml-test for all the new behaviours before we tag/release a new version. Once it's decided "yup, we've done everything we want to do for a new release" someone will need to spend some time on that (if someone hasn't already). There's a (Also: I assume that with "TOML 2.0" you mean "a new version"; I don't there will ever be an incompatible 2.0; the next version will most likely be 1.1).
That's up to you. I bet that >95% of TOML files are read by one application only. |
I'm talking about tools like linters and formatters. Their output is by definition meant to be read by other applications. However, the situation with newer TOML versions support in the ecosystem is dire: most libraries listed in the wiki still don't even support 1.0! I believe some written best practice advice is needed. |
That's not a good-faith read of the situation, methinks. Practically all of the actively-maintained implementations are v1.0.0-compliant. The implementations listed on the wiki under v0.5.0 and below are basically all dead/abandoned projects. |
I'm not sure how up-to-date the wiki is, plus many libraries listed there are unmaintained. For example for C♯ it lists:
So basically, all the v0.5.0-compliant ones are unmaintained. I just listed the C♯ ones because that happened to be the top one, but I bet the situation is similar for many other languages. Someone should probably clean up the wiki a bit. Other than that, I would say "use your best judgement". Different projects can make different reasonable decisions here: some might want to update to TOML 1.1 as soon as possible and that's completely fine, others might want to stick with 1.0 (or even 0.5) for a long time, and that's fine too. I don't think there is "one right way" to go about this. |
For what it's worth, my take is that the changes don't merit a v2.0 because they don't break valid v1.0.0 configs. There are still significant open PRs like #891 that are backwards compatible, so perhaps a v1.1.0 is worth considering? Output tools should write configs that the input tools can read. That's the be-all and end-all of it. Revisit the second question when parsers start to accept newlines in inline tables. |
This will be in a 1.1.0. I'm not gonna commit to a timeline for that tho.
Well, it depends. If they're style-preserving, sure -- preserve it. If they're regular dumps, and they can preserve 1.0 compat, it's reasonable too. I think it's reasonable to trust each parser/dumper author to figure out what makes sense for their library and how many knobs they wanna give their users. |
Hello, it's been more than a year since you said that, any news ? Thanks |
Well this PR was merged, but we're discussing TOML 1.1.0 over at #928. |
Fixes #516