refactoring errors & assignment

[chanced]

• Removes error, Error
• Adds AssignError
• Adds ResolveError
• Adds DeleteError
• Changes Assign, Resolve, ResolveMut, Delete traits to now use associated types for Value and Error
• Removes MaybePointer
• Removes Assignment
• Adds Pointer::split_at
• Changes Assign::assign to return the replaced value as Option<Assign::Value>

[chanced]

This isn't ready yet - i still have a bunch of problems in the tests to fix.

@asmello sorry this just now coming in and still not ready. I know you aren't blocked or in a hurry but I told you I was shooting for a Monday release.

I'm hoping to get it wrapped up either tonight or tomorrow.

I still need to:

• fix tests / code for failing tests & write a couple more
• Components iter - maybe.
• Refactor delete in the same fashion as assign
• Documentation

[chanced]

I went ahead and backed out snafu for hand-rolled errors. I'm leaving offset intact for now until we come up with the appropriate replacement.

[chanced]

I'm going to return a PointerBuf as part of Asssignment but it could be Cow<'p, Pointer> so that for most cases, it'd have the lifetime of the input Pointer but in the case of one that included "-", it'd be PointerBuf.

I dont know if its worth it. On one hand, more logic. On the other, i doubt "-" will show up much so most could be borrowed.

i'm running out of fumes tonight but I'll circle back to this in the morning.

[chanced]

@asmello hey, one of the quickcheck tests (pointer::tests::qc_intersection) is failing but i dont see anything obvious.

I'm not sure how I'm tripping it.

    #[quickcheck]
    fn qc_intersection(base: PointerBuf, suffix_0: PointerBuf, suffix_1: PointerBuf) -> TestResult {
        if suffix_0.first() == suffix_1.first() {
            // base must be the true intersection
            return TestResult::discard();
        }
        let mut a = base.clone();
        a.append(&suffix_0);
        let mut b = base.clone();
        b.append(&suffix_1);
        let isect = a.intersection(&b);
        TestResult::from_bool(isect == base)
    }

failures:
---- pointer::tests::qc_intersection stdout ----
thread 'pointer::tests::qc_intersection' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/quickcheck-1.0.3/src/tester.rs:165:28:
[quickcheck] TEST FAILED. Arguments: (PointerBuf(""), PointerBuf("/"), PointerBuf("/~0 :\u{2}‥%>ZXD~1 \u{1e}`㋯H\u{e}\u{8}\u{6dd}6!\u{94}:S,ゥS′?'@¡\u{98}\u{99}0'B\u{16}\u{ffff}\u{1a}\u{10fffd}v\u{e510}\u{2}"))

[chanced]

@asmello this method on Pointer is no longer used. I'm just checking to see if you have utility for it before I delete it:

    fn partial_path(&self, suffix: &Self) -> &Self {
        self.strip_suffix(suffix).expect("suffix came from self")
    }

[chanced]

I think I know what it is.

edit: fixed it, sorry about the mention. I wasn't sure why it randomly tripped and stayed consistent. I haven't used quickcheck before so it tripped me up.

[chanced]

Added more tests for parse, some of which are failing.

edit: done with that.

[chanced]

The only thing left that I wanted to do was add the Components iterator. It may be the best place to include the index, if we go with something other than offsets.

EDIT: oh, and documentation but I'm not sure how to write those yet.

for mod structure, I'm not sure which would be better:

flatten: flatten everything into lib.rs

partial flatten flatten these into lib.rs

mod tokens;
mod pointer;
mod token;
pub mod index;

make these pub and continue to re-export:

mod assign;
pub use assign::{Assign, AssignError, Assignment};
mod delete;
pub use delete::Delete;
mod resolve;
pub use resolve::{Resolve, ResolveError, ResolveMut};

restructure something to the effect of:

pub mod pointer;
pub use pointer::{ Pointer, PointerBuf, ParseError };

pub mod token;
pub use token::{Token, Tokens, Index};

pub mod assign;
pub use assign::{ Assign, AssignError, Assignment };

pub mod delete;
pub use delete::Delete;

pub mod resolve;
pub use resolve::{ Resolve, ResolveError, ResolveMut };

[chanced]

@asmello if you're good with this so far, I'll merge it in. I don't want to cut a release until we shake out all of the remaining breaking changes but this gets the logic mostly where it needs to be. There are some minor things, like restructuring the mods, Pointerbuf::Append accepting &Pointer or T: AsRef<Pointer>, and whether or not we change offsets to indexes.

[asmello]

Still catching up, think I'll need to do a proper review tomorrow as it's already getting late here. But I'll comment here and there.

for mod structure, I'm not sure which would be better

I'm not sure there are any official guidelines for this. There seems to be a bit of a philosophical disagreement, though there are some common points. I think we should avoid exporting the same symbol in more than one path (which I think I may have been guilty of with Index - let's fix that). Other than that, a good rule of thumb is probably to export the most commonly used symbols at the top-level, and leave the less used ones (and in particular the ones that users can't instantiate directly) accessible in nested modules.

What constitutes "commonly used symbols" is open to interpretation. Having used jsonptr a fair bit, I'm confident we can include Pointer, PointerBuf, Token and probably Error in that category. Index probably shouldn't, as it's primarily used anonymously.

I haven't really used the the assign/resolve/delete API much in practice, so I don't have much of an opinion on it, but it does seem to me like a less core component of the crate. May seem surprising, since the README examples focus on those APIs heavily, but beyond my own experience, the fact this API is not used in json_patch adds some weight to this view. Not to say we should remove that code, but perhaps it can be under a feature flag. Regardless, I think those symbols shouldn't be at the top level. They should probably still, however, be fairly high up. I think it'd be reasonable to export them all in common module. Not sure what to name it - maybe evaluation? A bit long though.

[asmello]

@asmello if you're good with this so far, I'll merge it in. I don't want to cut a release until we shake out all of the remaining breaking changes but this gets the logic mostly where it needs to be. There are some minor things, like restructuring the mods, Pointerbuf::Append accepting &Pointer or T: AsRef<Pointer>, and whether or not we change offsets to indexes.

If this is holding up other changes feel free to merge and I can review async. I agree with the general approach from what I skimmed over.

[chanced]

Still catching up, think I'll need to do a proper review tomorrow as it's already getting late here. But I'll comment here and there.

Sounds good. I'd rather wait until you've had a chance to run through it.

[asmello]

edit: fixed it, sorry about the mention. I wasn't sure why it randomly tripped and stayed consistent. I haven't used quickcheck before so it tripped me up.

Yeah no worries. I've had a very busy week, unfortunately, so apologies for not engaging much so far. Glad you figured out the issue. Quickcheck is a powerful tool to automate some edge case testing, but it takes some getting used to for interpretation.

[asmello]

@asmello this method on Pointer is no longer used. I'm just checking to see if you have utility for it before I delete it:

    fn partial_path(&self, suffix: &Self) -> &Self {
        self.strip_suffix(suffix).expect("suffix came from self")
    }

It's a private method, if it's no longer used, drop it ruthlessly. 😄

[chanced]

so apologies for not engaging much so far

Oh no need to apologize whatsoever! I feel bad that I said I would have this done on Monday and it is now Friday. It's been a week on this side too.

[chanced]

I haven't really used the the assign/resolve/delete API much in practice,

Yea, the way they are currently built just aren't useful at all. I have been chipping away at refactoring them in this PR but I've been doubting their usefulness. I have partially included a refactor in the last push. It isn't complete (AssignValue would need to be generic, and other traits would need to be updated).

The update would allow implementations for yaml, toml (through feature flags),or whatever else someone wanted to impl.

I'm actually cool with not doing this though and simply always accepting serde_json::Value and deleting the traits.

[chanced]

@asmello dude, go get some sleep. This can wait.

[asmello]

I haven't really used the the assign/resolve/delete API much in practice,

Yea, the way they are currently built just aren't useful at all. I have been chipping away at refactoring them in this PR but I've been doubting their usefulness. I have partially included a refactor in the last push. It isn't complete (AssignValue would need to be generic, and other traits would need to be updated).

The update would allow implementations for yaml, toml (through feature flags),or whatever else someone wanted to impl.

I'm actually cool with not doing this though and simply always accepting serde_json::Value and deleting the traits.

Oh I think this is all useful, especially post-refactor, what I mean is more that the basic Pointer type and companions is already useful on its own, so some users may want to consume just that part in isolation.

[chanced]

Oh I think this is all useful, especially post-refactor, what I mean is more that the basic Pointer type and companions is already useful on its own, so some users may want to consume just that part in isolation.

Oh, I see. I thought you meant the traits. They were mostly useless before. I'll finish this refactor then. We can decide whether to dump them or go with serde_json::Value.

I like the idea of making assign, resolve, and delete feature flags that are enabled by default.

[chanced]

Added a draft for possible solution of expansion.

[chanced]

I'm not super excited about the name of AutoExpand or the variants (Enabled, Disabled) but they work for on/off. If more are added, such as maybe "AllObjects" that always creates object paths irregardless of the token, then they'd need proper names.

I also need to revisit what is passed in. This was just the min to get it working. Current pointer (path up to and including the current token) should probably be provided as well. If so, a struct may be needed as two &Pointers as parameters may yield ambiguity as to which is which.

[asmello]

I'm not super excited about the name of AutoExpand or the variants (Enabled, Disabled) but they work for on/off. If more are added, such as maybe "AllObjects" that always creates object paths irregardless of the token, then they'd need proper names.

Disabled is probably fine, don't think it is invalidated if more are added, but how about Default for the other? Could always also tweak the meaning without it being a semantic break.

If so, a struct may be needed as two &Pointers as parameters may yield ambiguity as to which is which.

Sounds reasonable!

General comment: I think things are still a bit in flux (especially given the todo!() you added as part of the strategy refactor), but don't forget to run clippy before you're done, there's a few warnings I won't comment about. ;)

[asmello]

Done with a proper review! Generally LGTM, solid code! Mostly minor stuff. The amount of tests gives me confidence there's nothing too bad I may have missed. :joy:

BTW - seems like I can't resolve my own comments! :\

[chanced]

General comment: I think things are still a bit in flux (especially given the todo!() you added as part of the strategy refactor), but don't forget to run clippy before you're done, there's a few warnings I won't comment about. ;)

Yea, I didn't have time to flush it out but wanted to get it pushed incase you had a chance to look.

[chanced]

@asmello The more I worked on Assign, the more apparent it became that trying to provide a strategy for replacement just wasn't the right solution. Also dealing with the return of a mutable reference became a hassle.

I walked it back and deleted Assignment in favor of returning Option<Value> (replaced).

The possibility of ambiguity is too high for this to be used in anything that demands precision. I think there's a better solution for more controlled assignment, perhaps something like an iterator of entries. I'm not quite sure yet.

Either way, I don't think we need to solve it now. Neither of us are using this API and we haven't received feedback from anyone who is. I think it's best just to keep it simple with a helpful API that is only suitable for certain situations.

Let me know what you think. I'll work on adding tests and documentation in the meantime. I'll merge this in if you're cool with abandoning the effort.

[asmello]

@asmello The more I worked on Assign, the more apparent it became that trying to provide a strategy for replacement just wasn't the right solution. Also dealing with the return of a mutable reference became a hassle.

I walked it back and deleted Assignment in favor of returning Option<Value> (replaced).

The possibility of ambiguity is too high for this to be used in anything that demands precision. I think there's a better solution for more controlled assignment, perhaps something like an iterator of entries. I'm not quite sure yet.

Either way, I don't think we need to solve it now. Neither of us are using this API and we haven't received feedback from anyone who is. I think it's best just to keep it simple with a helpful API that is only suitable for certain situations.

Let me know what you think. I'll work on adding tests and documentation in the meantime. I'll merge this in if you're cool with abandoning the effort.

That works for me. I'm always in favour of keeping things simple if there's no clear impetus for added complexity.

[chanced]

Oh.. so I don't know how to handle including README.md as docs which has tests that depend on feature flags that are not enabled by default.

Should I enable "ops" (which enables "assign", "resolve", "delete") by default, just get rid of the flags entirely, or avoid including README.md?

[chanced]

Walking back the feature flags for this PR.

[chanced]

was about to hit Squash and merge but hit this:

---- pointer::tests::qc_intersection stdout ----
thread 'pointer::tests::qc_intersection' panicked at /Users/chance/.cargo/registry/src/index.crates.io-6f17d22bba15001f/quickcheck-1.0.3/src/tester.rs:165:28:
[quickcheck] TEST FAILED. Arguments: (PointerBuf(""), PointerBuf("/"), PointerBuf("/_\u{10fffd}\u{8d}¡5q^U~1S\u{3e1b3}*5\u{349fe}\u{2029}蔗|\u{d7938}\u{96}[),_峠턲\u{96}\u{858ef}\u{10e26d}帋Ϲ\u{17}R湤E\u{95}7g&\u{10}(\u{98284}㞩\u{e}|\u{8b}¯\u{86}‧;1\u{9fbb2}㓌\u{8}\u{a69da}¤S \u{92}4\u{9efc0}?\u{2068}N_\u{e001}\u{604}\u{10ffff}$?"))