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

Item::as_table is a beatrap #838

Closed
ijackson opened this issue Mar 5, 2025 · 3 comments · Fixed by #839
Closed

Item::as_table is a beatrap #838

ijackson opened this issue Mar 5, 2025 · 3 comments · Fixed by #839

Comments

@ijackson
Copy link

ijackson commented Mar 5, 2025

Item::as_table looks at first glance like the obvious method to call if you want to do a normal table value lookup. But it returns None for Item::Value(Value::InlineTable(..)).

The call you should use instead for this is as_table_like. That's a longer name (so less obvious). There's nothing in the documentation of as_table to warn you about this.

I was tripped up by this because the author of a crate that now uses toml_edit wrote the bug that this API induces. (I suspect from the history there that that project switched from another toml library, which presumably an .as_table function that gave Some for inline tables too.)

I suggest:

  1. Right away, improve the documentation for .as_table and .is_table (in Item)
  2. Next time we do an incompatible bump, rename {as,is}_table -> {as,is}_outline_table (or similar bikeshed) and {as,is}_table_like to {as,is}_table.
@epage
Copy link
Member

epage commented Mar 5, 2025

Next time we do an incompatible bump, rename {as,is}_table -> {as,is}_outline_table (or similar bikeshed) and {as,is}_table_like to {as,is}_table.

The current naming is very consistent {as,is}_{type}. We have Table, InlineTable, and dyn TableLike. The first two names are setup to match the TOML Specification.

@ijackson
Copy link
Author

ijackson commented Mar 6, 2025

The current naming is very consistent {as,is}_{type}. We have Table, InlineTable, and dyn TableLike. The first two names are setup to match the TOML Specification.

I see the value in consistency. I do think we should at least consider renaming them (and, therefore, renaming these types too). I think it's more important to provide an API that helps our users avoid foreseeable bugs, than to be 100% faithful with upstream terminology.

@epage
Copy link
Member

epage commented Mar 6, 2025

This is a low level API dealing with concepts as they exist in the TOML grammar. toml should be used if they want an API that deals only with high-leveler details.

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

Successfully merging a pull request may close this issue.

2 participants