Support dotted keys #73

ironyman · 2019-11-23T23:14:30Z

When a dotted key a.b.c is created as a child of table T, we create a dotted key marker in T and descend into child tables of T: a, b then create the dotted key a.b.c. The dotted key marker contains the path to the real dotted key. The display walker will print the the dotted key when it finds the dotted key marker by descending into the child tables according to the path in the dotted key marker.

Insert and remove works.

        // Remove works.
        root["a.b"] = value(3);
        assert!(root["a"]["b"].as_integer().unwrap() == 3);
        root.remove("a.b");
        
        // Repeated insert and remove works.
        root["a.c"] = value(2);
        root.remove("a.b");
        root["a.c"] = value("string");
        root.remove("a.b");
        root["a.c"] = value("derp");

Also sorting works.

ordian

I had a brief look and the general direction seems right. The thing we need to think about is what we'd like to expose in the public API to make it easy to use and ergonomic.

ordian · 2019-11-24T19:01:00Z

src/decor.rs

-#[derive(Eq, PartialEq, Clone, Debug, Hash)]
-pub(crate) struct Repr {
+#[derive(Eq, PartialEq, Ord, PartialOrd, Clone, Debug, Hash)]
+pub struct Repr {


is it used in pub API?

ordian · 2019-11-24T19:05:08Z

src/decor.rs

@@ -8,15 +8,15 @@ pub struct Formatted<T> {

 // String representation of a key or a value
 // together with a decoration.
-#[derive(Eq, PartialEq, Clone, Debug, Hash)]
-pub(crate) struct Repr {
+#[derive(Eq, PartialEq, Ord, PartialOrd, Clone, Debug, Hash)]


I don't think the derived ordering is "correct" here, it will first compare prefix, then suffixes, then raw_value

ordian · 2019-11-24T19:11:26Z

src/parser/key.rs

-    )))
+// From spec:
+// simple-key = quoted-key / unquoted-key
+// actual: key = unquoted-key / basic-string / literal-string


it was implemented using an older version of spec, so if it has changed, the comment is worth updating as well

You mean the code should be updated? I added the comment to remind us the parser branches differently than the spec but the result is the same.

I've adapted the spec in comments to the parser in some places, so sometimes it's not a direct copy-paste. From your comment change it's not clear whether they are equivalent, so it would be good to update the comment to clarify that.

ordian · 2019-11-24T19:14:45Z

src/table.rs

+    /// Type for display walker to know where (during walk) to
+    /// print dotted key. To get value, parse
+    /// the key and descend table tree.
+    DottedKeyMarker(Vec<SimpleKey>),


i don't like that the internal implementation detail is leaking into the public API

Should we make Item opaque to the user?

Hmm, I'm not sure. Currently, it's possible to delete an item using table['key'] = Item::None and I'd like to avoid unnecessary breaking changes unless there is a good reason for that. Again, let's think about what kind of API we'd like to expose the user (primarity cargo-edit), and go from that.

ironyman · 2021-01-10T23:46:13Z

I'm not sure what the others' positions are on this. I thought @ordian wanted some time to figure out how we should do the public API for manipulating dotted keys. And some folks in cargo-edit wanted to try another toml library. So I'm not sure where we are.

ordian

This PR contains some uncommented code and unresolved TODOs. I'd like it to be resolved before it's merged.

Also not sure if it matches the newest spec
https://toml.io/en/v1.0.0-rc.3#keys

ordian · 2021-01-11T08:44:19Z

src/key.rs

@@ -26,11 +27,36 @@ use std::str::FromStr;
 ///
 /// To parse a key use `FromStr` trait implementation: `"string".parse::<Key>()`.
 #[derive(Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Clone)]
-pub struct Key {
+pub struct SimpleKey {


How about SingleKey? Also the doccomments need to be updated.

I was thinking of going a different direction and having Key and KeyPath or something.

One thing benefit of "SimpleKey" is that this is the term the grammar uses

ordian · 2021-01-11T08:45:12Z

src/key.rs

    raw: InternalString,
+    pub(crate) parts: Vec<SimpleKey>,


do we need to store raw representation or is it always constructible from the parts?

ordian · 2021-01-11T08:47:08Z

src/key.rs

+    }
+
+    /// Returns the parsed key value.
+    pub fn get(&self) -> InternalString {


the name InternalString suggests it's not for public API, we can return a String instead, but I'd prefer a IntoString impl or something like that.

ordian · 2021-01-11T08:47:35Z

src/key.rs

+        let keys_parts: Vec<_> = self.parts.iter().map(|k| k.raw.clone()).collect();
+        keys_parts.join(".")
+
+        // same as raw()?


seems like it

ordian · 2021-01-11T08:48:09Z

src/key.rs

+    /// Returns the key raw representation.
+    pub fn raw(&self) -> &str {
+        &self.raw
+    }
+
+    /// Get key path.
+    pub fn get_key_path(&self) -> &[SimpleKey] {
+        &self.parts
+    }
+
+    /// Get key path.
+    pub fn get_string_path(&self) -> Vec<InternalString> {
+        self.parts.iter().map(|r| r.key.clone()).collect()
+    }


I'm not convinced we need this.

ordian · 2021-01-11T08:50:55Z

src/table.rs

+    /// Type for display walker to know where (during walk) to
+    /// print dotted key. To get value, parse
+    /// the key and descend table tree.
+    DottedKeyMarker(Vec<SimpleKey>),


This is public API, if we do table['key'] = Item::DottedKeyMarker(vec!['a'.parse().unwrap(), 'b'.parse().unwrap()]);
what would it yield?
This part is the biggest concern with this PR.

ordian · 2021-01-11T08:55:35Z

src/table.rs

+                    Item::DottedKeyMarker(parsed_key.parts.clone())
+                ));
+            TomlParser::descend_path(self, &path[..path.len() - 1], 0, true)
+                .expect("the table path is valid; qed")


hmm, looking at this function it always return Ok (or panics).
Looks like something was lost in a refactoring.

ordian · 2021-01-11T08:57:47Z

tests/test_edit.rs

+        root["d.b.d"] = value(1);
+        root["c.b.d"] = value(1);
+        root["b.b.d"] = value(1);
+        root["a.'c'.d"] = value(3);


would be good to also test something like

site."google.com" = true

ironyman and others added 10 commits October 2, 2019 23:01

Support dotted keys

b843fe7

Inserting sort of works

fc4f0cc

I broke something

9b84a51

Sorting doesn't work without full key path in Table.items

649ba4b

Will add marker for dotted key

bf140a1

Tests pass!

0abd56b

Add remove tests

2357eda

Clean up debagging stuff

cca2ec3

Add todo

644455d

Update test

fafbdee

ordian reviewed Nov 24, 2019

View reviewed changes

andrewhickman mentioned this pull request Sep 3, 2020

Dotted keys are not supported #91

Closed

6 tasks

This comment has been minimized.

Sign in to view

ordian reviewed Jan 11, 2021

View reviewed changes

ordian mentioned this pull request Feb 5, 2021

add support for toml 1.0 #58

Closed

3 tasks

bearcage mentioned this pull request May 17, 2021

Maturin's toml parser errors out on dotted keys in Cargo.toml when building sdists PyO3/maturin#541

Closed

epage mentioned this pull request Aug 23, 2021

Iterating on toml_edits API #122

Closed

ordian closed this Sep 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support dotted keys #73

Support dotted keys #73

ironyman commented Nov 23, 2019

ordian left a comment •

edited

Loading

ordian Nov 24, 2019

ordian Nov 24, 2019

ordian Nov 24, 2019

ironyman Nov 24, 2019

ordian Nov 25, 2019

ordian Nov 24, 2019

ironyman Nov 24, 2019

ordian Nov 25, 2019

This comment has been minimized.

ironyman commented Jan 10, 2021

ordian left a comment

ordian Jan 11, 2021

epage Aug 20, 2021

ordian Jan 11, 2021

ordian Jan 11, 2021

ordian Jan 11, 2021

ordian Jan 11, 2021

ordian Jan 11, 2021

ordian Jan 11, 2021

ordian Jan 11, 2021

Support dotted keys #73

Support dotted keys #73

Conversation

ironyman commented Nov 23, 2019

ordian left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

ironyman commented Jan 10, 2021

ordian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian left a comment •

edited

Loading