refactoring errors & assignment #41

chanced · 2024-06-19T21:41:59Z

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 · 2024-06-19T21:47:45Z

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

asmello

Just did a quick, non-exhaustive look to check the approach and added comments at random places. Will try to do a proper review ASAP, hopefully this Friday, if you have it ready by then.

Cargo.toml

asmello · 2024-06-19T22:10:44Z

src/assign.rs

-use serde_json::Value;
+use core::mem::replace;

-use crate::{Error, Pointer, PointerBuf};
+use serde_json::{map::Entry, Map, Value};
+use snafu::ResultExt;
+
+use crate::{assign_error, AssignError, Pointer, Token};


We can have a separate discussion about this, but worth converging on a style for imports. I generally prefer to just merge everything into a single block, have never found it useful to separate them, but if you feel strongly against it, I can accept this convention too.

Oh, I have no preference here. I'm lazy and let RA just do its thing. I'm definitely game for cleaning them up. I think there's even a lint that'll do it but its in nightly and was hit-or-miss for me when I tried it last.

I think you're right. It's a bit unfortunate RA doesn't do exactly what I want by default, and there's no setting for it, but if you just merge the blocks once it'll keep them merged (even if you add more imports later). No need to consolidate this everywhere in this PR, but might as well start with it for new code.

I think the untidiness of imports is amplified by all the unnecessary mods in the crate. Flattening it should make this a lot easier to keep in check.

I may try that lint again too. I was rather excited when I found it and super disappointed when it had some issues. While I find the utility of auto-imports super convenient, I do get annoyed and distracted by cluttered use blocks.

src/assign.rs

src/pointer.rs

src/error.rs

chanced · 2024-06-20T16:41:26Z

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 · 2024-06-21T06:44:55Z

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 · 2024-06-21T13:53:58Z

@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 · 2024-06-21T15:09:27Z

@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 now simply calls A::assign

chanced · 2024-06-21T17:31:59Z

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 · 2024-06-21T18:13:55Z

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

edit: done with that.

chanced · 2024-06-22T13:05:14Z

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 · 2024-06-22T13:37:06Z

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. ;)

Cargo.toml

src/assign.rs

src/pointer.rs

asmello · 2024-06-22T14:06:49Z

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. 😂

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

chanced · 2024-06-23T12:56:31Z

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 · 2024-06-24T14:33:04Z

@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 · 2024-06-24T15:29:58Z

@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.

.taplo.toml

src/assign.rs

chanced · 2024-06-24T15:52:24Z

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 · 2024-06-24T15:59:56Z

Walking back the feature flags for this PR.

chanced · 2024-06-24T16:29:53Z

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}$?"))

refactoring errors & assignment

4ef7791

chanced added 2 commits June 19, 2024 18:04

fixes assign offset

3ffd4eb

fixes creation of object in assignment

ace6d24

asmello reviewed Jun 19, 2024

View reviewed changes

backs out snafu

c4566e0

chanced added 4 commits June 20, 2024 12:44

quick cleanup

b590a2e

fixes compiler errors in commented out tests - 4 tests still failing

c8cc90d

refactoring but not done

4bdd530

assignment wip

91310fa

chanced added 5 commits June 21, 2024 02:58

assignment refactor is close but still needs work

62f8a5c

fixes pathing issues in assignment

05b1acd

still wip

b3fc0c4

fixes assign

19172f0

removes println

9ee3c6b

chanced marked this pull request as ready for review June 21, 2024 13:54

chanced added 2 commits June 21, 2024 10:52

removes error.rs

ba2cc31

minor docs

bb509af

chanced added 4 commits June 21, 2024 11:15

Pointer::assign now accepts &mut A where A: Assign

825d426

it now simply calls A::assign

moves resolve tests

87343b6

adds TODOs to lib.rs

3be8724

adds tests for delete

f22f9fa

chanced added 2 commits June 21, 2024 13:38

fixes parsing of "/" pointer

9a242c5

adds more parse tests, some of which are failing

24ca9e6

asmello reviewed Jun 22, 2024

View reviewed changes

chanced added 8 commits June 23, 2024 10:45

changes the signature of Assign::assign to accept new Expansion enum

a37bf1f

fixes expect message

89c541a

refactors and finishes the first draft of Expand

da30d49

fixes tests

6043341

fixes doc tests

c6a4fea

style change of Cargo.toml, prep for insta

d729a3f

removes yaml feature flag. no longer planning to support

1049960

walks back Expansion & Expand, removes Assignment

59ce3c3

removes FailedToExpand from AssignError

2d1fcf3

asmello reviewed Jun 24, 2024

View reviewed changes

.taplo.toml Outdated Show resolved Hide resolved

src/assign.rs Outdated Show resolved Hide resolved

chanced added 4 commits June 24, 2024 11:41

walks back taplo style

5f8f3de

spacing

e227b1f

fixes build

23e2fd1

spacing

7987a3c

chanced added 3 commits June 24, 2024 12:06

walks back ops feature flags

f212e1d

fixes doc tests

4f5f31d

CHANGELOG

d2a6def

adds skeleton for failing test to demonstrate issue #44

b0fbc91

chanced merged commit bd92284 into main Jun 24, 2024
1 check failed

chanced mentioned this pull request Jun 24, 2024

updates CHANGELOG.md #47

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactoring errors & assignment #41

refactoring errors & assignment #41

chanced commented Jun 19, 2024 •

edited

Loading

chanced commented Jun 19, 2024 •

edited

Loading

asmello left a comment

asmello Jun 19, 2024

chanced Jun 19, 2024 •

edited

Loading

asmello Jun 20, 2024

chanced Jun 20, 2024 •

edited

Loading

chanced commented Jun 20, 2024

chanced commented Jun 21, 2024

chanced commented Jun 21, 2024 •

edited

Loading

chanced commented Jun 21, 2024

chanced commented Jun 21, 2024 •

edited

Loading

chanced commented Jun 21, 2024 •

edited

Loading

chanced commented Jun 22, 2024 •

edited

Loading

asmello commented Jun 22, 2024

asmello commented Jun 22, 2024 •

edited

Loading

chanced commented Jun 23, 2024

chanced commented Jun 24, 2024 •

edited

Loading

asmello commented Jun 24, 2024

chanced commented Jun 24, 2024 •

edited

Loading

chanced commented Jun 24, 2024

chanced commented Jun 24, 2024 •

edited

Loading

refactoring errors & assignment #41

refactoring errors & assignment #41

Conversation

chanced commented Jun 19, 2024 • edited Loading

chanced commented Jun 19, 2024 • edited Loading

asmello left a comment

Choose a reason for hiding this comment

asmello Jun 19, 2024

Choose a reason for hiding this comment

chanced Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

asmello Jun 20, 2024

Choose a reason for hiding this comment

chanced Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

chanced commented Jun 20, 2024

chanced commented Jun 21, 2024

chanced commented Jun 21, 2024 • edited Loading

chanced commented Jun 21, 2024

chanced commented Jun 21, 2024 • edited Loading

chanced commented Jun 21, 2024 • edited Loading

chanced commented Jun 22, 2024 • edited Loading

asmello commented Jun 22, 2024

asmello commented Jun 22, 2024 • edited Loading

chanced commented Jun 23, 2024

chanced commented Jun 24, 2024 • edited Loading

asmello commented Jun 24, 2024

chanced commented Jun 24, 2024 • edited Loading

chanced commented Jun 24, 2024

chanced commented Jun 24, 2024 • edited Loading

chanced commented Jun 19, 2024 •

edited

Loading

chanced commented Jun 19, 2024 •

edited

Loading

chanced Jun 19, 2024 •

edited

Loading

chanced Jun 20, 2024 •

edited

Loading

chanced commented Jun 21, 2024 •

edited

Loading

chanced commented Jun 21, 2024 •

edited

Loading

chanced commented Jun 21, 2024 •

edited

Loading

chanced commented Jun 22, 2024 •

edited

Loading

asmello commented Jun 22, 2024 •

edited

Loading

chanced commented Jun 24, 2024 •

edited

Loading

chanced commented Jun 24, 2024 •

edited

Loading

chanced commented Jun 24, 2024 •

edited

Loading