Adds TOML support, refactors tests, adds `Deserialize` to `Pointer`

[chanced]

• Adds TOML support
• Implements Deserialize for `Pointer
• Refactors tests for assign, resolve, delete
• Fixes #44

[chanced]

I can't figure this one out.

enabling "toml" breaks the build after I added assign::toml because:

impl PartialOrd<String> for Pointer {
    fn partial_cmp(&self, other: &String) -> Option<Ordering> {
        self.0.partial_cmp(other)
    }
}

error[E0277]: can't compare `str` with `std::string::String`
   --> src/pointer.rs:539:28
    |
539 |         self.0.partial_cmp(other)
    |                ----------- ^^^^^ no implementation for `str < std::string::String` and `str > std::string::String`
    |                |
    |                required by a bound introduced by this call
    |
    = help: the trait `PartialOrd<std::string::String>` is not implemented for `str`
    = help: the following other types implement trait `PartialOrd<Rhs>`:
              <&'a str as PartialOrd<winnow::stream::BStr>>
              <&'a str as PartialOrd<winnow::stream::Bytes>>
              <str as PartialOrd<winnow::stream::BStr>>
              <str as PartialOrd<winnow::stream::Bytes>>
              <str as PartialOrd>

I'm assuming its a red herring but it is the only compiler error. If I comment out assign::toml it builds. But I don't know why. I've tried removing the feature flag "json", making serde_json a non-optional dependency.

I've made the feature flag "toml" enable "std" as well as it does not have a way to operate in a no_std env.

[chanced]

@asmello I added toml support but the only way I could get it to compile is commenting out impl PartialOrd<String> for Pointer. I cannot figure out why its causing the build to break.

(see comment above)

[asmello]

Looking into the PartialOrd thing, but I noticed a lot of clippy errors due to #[deny(clippy::pedantic)]. Worth fixing them before merging.

EDIT: I think the assign::toml thing is entirely a red herring, the error doesn't go away if I comment it out. But the fix is simple:

impl PartialOrd<String> for Pointer {
    fn partial_cmp(&self, other: &String) -> Option<Ordering> {
        self.0.partial_cmp(other.as_str())
    }
}

Also probably a good idea to make CI run cargo clippy.

[chanced]

Awesome, thanks!

Hmm, I wonder why rust analyzer isn't picking up the pedantic lints.

Re: ci, will do. I'm not very experienced with rust ci yet but I needed to update it anyway to handle feature flags and ideally add caching.

[asmello]

Hmm, I wonder why rust analyzer isn't picking up the pedantic lints.

I think you need to enable it with: "rust-analyzer.checkOnSave.command": "clippy"

[chanced]

Yea, that was it. Thanks!

On Wed, Jun 26, 2024 at 3:48 PM André Mello @.***> wrote:

Hmm, I wonder why rust analyzer isn't picking up the pedantic lints.

I think you need to enable it with: "rust-analyzer.checkOnSave.command": "clippy"

— Reply to this email directly, view it on GitHub https://github.com/chanced/jsonptr/pull/48#issuecomment-2192507946, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIGFZCTFJE4JOAD4CWXPD3ZJMLIBAVCNFSM6AAAAABJ4NIKKKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJSGUYDOOJUGY . You are receiving this because you authored the thread.Message ID: @.***>

[chanced]

@asmello can i get you to review the usage of insta in assign when you have time, please?

I keep running into strange, inconsistent snapshots. I wonder if that's because of the tables, maybe? Is there a better approach to handling it?

data:
  foo:
    - bar: baz
    - bar: qux
ptr: /foo/-/bar
assign: quux
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
    0       │-{"foo":"baz"}
          0 │+{"foo":[{"bar":"baz"},{"bar":"qux"},{"bar":"quux"}]}
────────────┴───────────────────────────────────────────────────────────────────

I'm not crazy about the formatting of data but I can live with it.

[chanced]

I think the assign::toml thing is entirely a red herring, the error doesn't go away if I comment it out. But the fix is simple:

I bet that was because when I was testing it, I only had assign done. I wonder if resolve and delete being present made it so that the error remained even if assign::toml was commented out.

I tried various approaches to get it to deref but didn't think to use as_str.

Thank you for figuring it out. That was a weird one.

[asmello]

I'm not crazy about the formatting of data but I can live with it.

I guess that's because it becomes part of the metadata written to the snapshot header, which is YAML. That's unfortunately not configurable.

As for the body/expression, you can use a different serialiser to get a more pretty-printed output, so there's at least more options.

[chanced]

@asmello i had to remove the tables for it to work. Tests in assign have been updated.

[chanced]

Well.. I thought it was fixed. CI is showing failures. Hmmm..

[asmello]

@asmello i had to remove the tables for it to work. Tests in assign have been updated.

That's... unexpected. It should work fine with the table tests. I've done it like that before.

Well.. I thought it was fixed. CI is showing failures. Hmmm..

I think the snapshots are just out of date, you need to regenerate them.

[chanced]

It's still hitting weird inconsistencies, for example:

The discrepancy between new and old is weird. The old snapshot is the result from comparing the replaced result while the new is comparing data.

I've run it a few times. I'll delete the snapshots and try again.

[asmello]

Oh interesting, I wonder if it's having trouble assigning the outputs to a number deterministically. I think you were calling the tests in a loop right? I recall there was a fix for that.

[chanced]

I was, which I assumed was the problem. I've since changed it so that they aren't called from a loop. However, the calls to assert_snapshot(data) and assert_debug_snapshot(result) are happening in a fn now. Maybe that's causing issues too?

        #[derive(Serialize, Debug)]
        struct Test {
            data: Value,
            ptr: &'static str,
            assign: Value,
        }

        fn test(data: impl Into<Value>, ptr: &'static str, assign: impl Into<Value>) {
            let mut data = data.into();
            let assign = assign.into();

            let test = Test {
                ptr,
                data: data.clone(),
                assign: assign.clone(),
            };
            let ptr = Pointer::from_static(test.ptr);
            let replaced = ptr.assign(&mut data, assign);
            insta::with_settings!({
                info => &test,
                description => "asserting that the JSON data is as expected after assignment",
                omit_expression => true // do not include the default expression
            }, {
                assert_snapshot!(&data);
            });
            insta::with_settings!({
                info => &test,
                description => "asserting that the replaced JSON data returned from the assignment is as expected",
                omit_expression => true // do not include the default expression
            }, {
                assert_debug_snapshot!(&replaced);
            });
        }

where tests are executed serially:

            test(json!({}), "/foo", json!("bar"));
            test(json!({"foo": "bar"}), "", json!("baz"));
            test(json!({"foo": "bar"}), "/foo", json!("baz"));
            test(json!({"foo": "bar"}), "/foo/bar", json!("baz"));

[asmello]

Yeah, it's weird that still fails. Maybe it's clashing with the old outputs somehow? I'd expect deleting and starting over to fix it in that case.

The way to make it work with loops is similar to their recipe for rtest: it's using set_snapshot_suffix.

BTW, I'm sorry this is causing you trouble. 😓 I've never had a hard time with insta... Do feel free to drop it if you get frustrated.

[chanced]

Using simple indexes didn't fly.

It's definitely because they are being called from a fn rather than inline. I guess it'd need a unique name for the test set + an index. The "Snapshot file" suggests to me they are colliding:

[chanced]

BTW, I'm sorry this is causing you trouble. 😓 I've never had a hard time with insta... Do feel free to drop it if you get frustrated.

Oh, it's no worries! New tools in this industry often come with a bit of frustration :). Also, this is very much seems to be caused by me not using it properly.

Here is the updated test fn:

fn test(idx: usize, data: impl Into<Value>, ptr: &'static str, assign: impl Into<Value>) {
    let mut data = data.into();
    let assign = assign.into();

    let test = Test {
        ptr,
        data: data.clone(),
        assign: assign.clone(),
    };
    let ptr = Pointer::from_static(test.ptr);
    let replaced = ptr.assign(&mut data, assign);
    insta::with_settings!({
        snapshot_suffix => format!("{idx}-data"),
        info => &test,
        description => format!("case #{idx}: asserting that the JSON data is as expected after assignment"),
        omit_expression => true // do not include the default expression
    }, {
        assert_snapshot!(&data);
    });
    insta::with_settings!({
        info => &test,
        snapshot_suffix => format!("{idx}-result"),
        description => format!("case #{idx}: asserting that the replaced JSON data returned from the assignment is as expected"),
        omit_expression => true // do not include the default expression
    }, {
        assert_debug_snapshot!(&replaced);
    });
}

[chanced]

@asmello

I went ahead and removed insta, rewrote a number of the tests to conform to a single approach, and refactored the mod structure of assign, delete, resolve a bit.

I was able to get it insta to work with a macro_rules but its just not really my cup of tea, at least not right now.

Hopefully this makes it a bit less cluttered and distracting.

[asmello]

That's alright, thanks for giving it a try and I'm sorry it didn't pan out.

I'll give it another look, but feel free to merge when you feel ready.