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

Allow advanced lookups in lookup and lookup_mut #89

Merged
merged 7 commits into from Mar 28, 2016

Conversation

kstrafe
Copy link
Contributor

@kstrafe kstrafe commented Mar 27, 2016

These commits allow one to search for tables with unconventional names. Example:

let table: toml::Value = r#"[table."name.other"] value = 0"#.parse().unwrap();
let look = table.lookup(r#"table."name.other".value"#).unwrap();

These commits have some breaking changes for the tests, as sending a raw " into lookup will no longer work correctly. One must send r#""\"""# or "\"\\\"\"" instead.

Performance: It may be possible to have a slight performance decrease for all lookups because each entry is now parsed and put into a String. This can be remedied by creating a lookup_simple method that takes simple names only.

Bugs discovered: the parser fails to parse "" and '' as a key name.

The new algorithm allows the explicit usage of "" and '' to denote key
names. This is useful for accessing tables or keys that are named in a
non-conventional manner.
@alexcrichton
Copy link
Collaborator

Thanks! Can you add some negative tests for lookup as well for when parsing fails?

@@ -290,6 +290,46 @@ impl<'a> Parser<'a> {
}
}

// Parse an array index as a natural number
fn array_index(&mut self) -> Option<String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there's a method to parse integers somewhere else, perhaps that could be leveraged here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think parsing integers accepts the +xx and -xx notation, which I don't think should be applicable. I'll need to look through the parser methods...
Edit: number_or_datetime can be used whilst evaluating the Value type to be an integer. I'll try leveraging it.
Edit2: fn integer may be the solution with allow_sign = false 👍

@kstrafe
Copy link
Contributor Author

kstrafe commented Mar 28, 2016

Added a bunch of negative tests and some positive ones.

I tried avoiding #[should_panic] as these tests easily break down if the toml is parsed wrong.

I'm a little uncertain of what convention to use in the tests.

@alexcrichton
Copy link
Collaborator

Looks great to me, thanks!

@alexcrichton alexcrichton merged commit c53fceb into toml-rs:master Mar 28, 2016
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

2 participants