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

Trouble with non-existing fields #299

Closed
f-saez opened this issue Jan 13, 2022 · 7 comments
Closed

Trouble with non-existing fields #299

f-saez opened this issue Jan 13, 2022 · 7 comments
Labels
C-question Uncertainty is involved

Comments

@f-saez
Copy link

f-saez commented Jan 13, 2022

Hi,

fn main() {
    let toml = r#"[package]
    name = "test"
    version = "0.1.0"
    edition = "2021"
     "#;

    let doc = toml.parse::<Document>().expect("invalid doc");
    dbg!(&doc["package"]["resolver"].is_none());
}

This code panic with message 'index not found', meaning it panic before evaluating is_none()

Same code worked well with toml_edit = "0.6.0" :

[src/main.rs:12] &doc["package"]["resolver"].is_none() = true

It stopped working with 0.7 to 0.12.6

If it's not a bug, how can I easily test if a field exists ?

thanks,

@ordian
Copy link
Member

ordian commented Jan 13, 2022

Use get instead of indexing. The previous behavior was changed in #240 to match indexing semantics to std libs.

@ordian
Copy link
Member

ordian commented Jan 13, 2022

@ordian ordian added the C-question Uncertainty is involved label Jan 13, 2022
@epage
Copy link
Member

epage commented Jan 13, 2022

I was just running into fighting this with upgrading cargo-edit from toml_edit v0.4.

I just released 0.13 which improves some consistency with this which made porting easier for me.

I'm overall torn on this. In some aspects, always dealing with Item::None (ie pre-0.7) makes the indexing easily chainable at the cost of being unlike the rest of Rust, unintentionally inserting tables, being sloppier in error handling, inconsistencies between Table and InlineTable because there isn't a Value::None, and extra internal complexity.

Right now we are in a mixed state where we deal with Item::None with IndexMut which simplifies inserting a deeply nested item (would involve a lot of entry calls). The main problems with this is InlineTables IndexMut can't participate and Item::get returns Item::None, making it inconsistent with Table::get.

I have a branch that explores switching to be more like the Rust stdlib. It greatly simplifies the code and better matches expectations at the cost of extra complexity in some cases for users.

@f-saez
Copy link
Author

f-saez commented Jan 13, 2022

thanks for the explanation.

I should have read the changelog more carefully.

@f-saez f-saez closed this as completed Jan 13, 2022
@ordian
Copy link
Member

ordian commented Jan 13, 2022

I just realized that the example from README.md is broken now. Would be good to fix that as well.

The original Index implementation was inspired by serde-json
https://github.com/serde-rs/json/blob/master/src/value/index.rs
and wasn't consistent with stdlib on purpose.

I agree about the downsides of this approach and am wondering what are we trading off here in the API ergonomics with more stdlib API/semantics.

@epage
Copy link
Member

epage commented Jan 13, 2022

I've posted what work I've done as #301 so others can play with it. The part that gave me pause as I was working on it was &mut table[key1][key2] type stuff because that would be table.entry(key1).or_insert_with(|| table()).entry(key2).or_insert_with(|| rable()).

@f-saez
Copy link
Author

f-saez commented Jan 13, 2022

doc["package"]["resolver"]["random_field"]

is easier to write and to deal with, and maybe I'm also used to that kind of writing (JSON crates), but it has it's drawback.

if it returns None, you don't know what part of the "path" is wrong.

As far as I'm concerned, the new writing suits me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-question Uncertainty is involved
Projects
None yet
Development

No branches or pull requests

3 participants