Adds fluid methods to Pointer

[chanced]

solves #31 by adding fluid methods to Pointer:

• with_leading_token - prepends a token
• with_trailing_token - appends a token
• with_appended - appends a pointer

[chanced]

@asmello do you have any opinion on the names? I struggled with them.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 31.81818% with 15 lines in your changes missing coverage. Please review.

Project coverage is 98.5%. Comparing base (a2453aa) to head (e57523d).

Files with missing lines	Patch %	Lines
src/pointer.rs	31.8%	15 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
src/pointer.rs	96.7% <31.8%> (-1.4%)	⬇️

... and 5 files with indirect coverage changes

[asmello]

I'm not convinced this is the right approach. I feel like .to_buf() is something that should always be an explicit step, especially when chaining, as that makes intermediate types less apparent.

Which potentially means the fluent API should exist in PointerBuf instead, if at all.

[chanced]

This is consistent with std::path::Path, e.g. Path::with_file_name

[asmello]

Fair point. I guess paths have some special semantics that make them more like a struct than a collection.

It's kinda interesting, not sure the std::path::Path design is very good, as path.with_extension("foo").with_extension("bar") results in two allocations, even though it doesn't really need to. We'll be inheriting that quirk. But then you're not really supposed to use this multiple times, so maybe that's ok.

I'm happy with the first two names — they're pretty descriptive, a bit on the long side but I think we don't want to encourage using this API too much, given the potential for allocation abuse, so that's actually good. I think we can do better with the last one though. How about .concat()?

EDIT

It would make sense to call it .join() to match std::path::Path, but then that method has special semantics we don't really match, so that could be potentially misleading. .concat() is unfortunately claimed by slice with a slightly different semantic, but then the same applies to slice::join so I think we can ignore that clash. Closest we have in terms of API is Iterator::chain, but chain is strongly associated with iterators so I don't think it fits well here. Same applies with extend.

So perhaps ending_with is a better alternative if we want to avoid std baggage. Then could also use ending_with_token to match (or do the reverse and use with_trailing for the append operation).

[chanced]

Yea, hopefully folks won't shoot themselves in the foot with it. I'm hoping the ergonomics of it are worth it. Maybe I should add a note about how they allocate and that they above should be avoided.

Oh, I like concat. If we go with it, does it make sense to implement std::slice::Concat?

Edit
Ah, I just noticed your edit.

[asmello]

Maybe I should add a note about how they allocate and that they above should be avoided.

Definitely! This is well documented in the Path methods for a reason ;)

If you're happy with .concat() I think that's fine. Just wanted to provide another alternative.

If we go with it, does it make sense to implement std::slice::Concat?

I don't think so, for one that trait is nightly and for another its API is different, we'd need to use it like [ptr1, ptr2].concat() which doesn't seem very discoverable (or particularly ergonomic). I also suspect it's not really meant to be used outside of slices, docs say "Helper trait for [T]::concat."

[chanced]

Ah, okay. I wasn't exactly certain how the call looked. From the trait, it seemed straight forward in that it added a concat method which returned Self::Output. Didn't notice it was nightly either.

I'll rename to concat and add docs. This is likely a silly change but I'm dealing with pointers a lot and all mutation is basically clone & then push a token.

Then again.. maybe its not worth the savings in loc.

[asmello]

Yeah it's fine to try this out, especially given the std::path precedent. The crate is not 1.0 yet so no need to overthink it. Let's just make sure the allocation is well documented and see if we end up making mistakes. 👍

[asmello]

Minor nit, but I think impl Into<Token<'_>> would be cleaner (unsure about whether the lifetime can be elided but I think so)

[asmello]

Just a thought, but curious how you start with &Pointer and end up having to extend it — could you perhaps start with PointerBuf and just reuse it throughout? Quite possible you need a copy, but on the off chance you don't, that'd be an interesting data point.

I'm thinking of some network coding I've been doing where I found that what works best is to allocate a single permanent buffer at a high level, sift it down the abstraction layers up to the point where I do a socket read, and then on the way back up all the parsing (well, most of it) can be done with zero copies. It's quite neat from a lifetime management perspective, but most importantly it helps monstrously with reducing allocations. It's quite easy and tempting to allocate a fresh buffer for every read (or every read site) but that's also easily ends up being a bottleneck. Even if it's a local (but permanent) buffer, you end up having to copy on the way up.

[chanced]

Nah, I can't reuse the buffer as they get stored.

This is a function that resolves a source and inserts it into my source database :

async fn resolve_path<'int, R: 'static + Resolve>(
    sources: &'int mut Sources,
    resolver: &R,
    uri: &AbsoluteUri,
    base: AbsoluteUri,
    fragment: String,
) -> Result<Resolved, Error<R>> {
    let resolved_root = resolve_root(sources, resolver, &base).await?;
    let located = resolved_root.located;
    let document_key = resolved_root.document_key;

    let relative_path = Pointer::parse(&fragment).map_err(|source| Error::InvalidPointer {
        uri: uri.clone(),
        source,
    })?;

    let mut absolute_path = sources.source(resolved_root.source_key).path().to_buf(); 
    absolute_path.append(&relative_path);

    let source_key = sources
        .link(New {
            uri: uri.clone(),
            document_key,
            absolute_path,
            fragment: Some(Fragment::Pointer(relative_path.to_buf())),
        })
        .map_err(|source| match source {
            LinkError::InvalidPath(err) => Error::PathNotFound {
                uri: uri.clone(),
                source: err,
            },
            LinkError::SourceConflict(err) => unreachable!(
                "\n\nsource conflict:\n\nuri: {uri}\ndocument_key: {document_key:?}\nerr: {err}",
            ),
        })?;

    Ok(Resolved::Source(resolved::Src {
        link_created: true,
        source_key,
        document_key,
        located,
    }))
}

https://github.com/chanced/grill/blob/0d3e2bfc4505d5638717b55826bf8ae6c2a5fc83/grill-json-schema/src/compile/scan/resolve.rs#L99-L139

[asmello]

Well, fair enough. Was worth a shot.

[chanced]

Absolutely, and I appreciate it.

I updated the docs to reflect std with an additional note that may be a bit too much.

[chanced]

@asmello (sorry, disregard) I tried using the inline, t: impl syntax but I'm running into type ambiguity with it:

    pub fn push_back<'t, T>(&mut self, token: impl Into<Token<'t>>) {
        self.0.push('/');
        self.0.push_str(token.into().encoded());
    }
    pub fn with_trailing_token<'t>(&self, token: impl Into<Token<'t>>) -> PointerBuf {
        let mut buf = self.to_buf();
        buf.push_back(token.into());
        buf
    }

▶ cargo check
    Checking jsonptr v0.6.0 (/Users/chance/dev/jsonptr)
error[E0282]: type annotations needed
   --> src/pointer.rs:442:13
    |
442 |         buf.push_back(token.into());
    |             ^^^^^^^^^ cannot infer type for type parameter `T` declared on the method `push_back`

error[E0282]: type annotations needed
   --> src/pointer.rs:462:13
    |
462 |         buf.push_front(token);
    |             ^^^^^^^^^^ cannot infer type for type parameter `T` declared on the method `push_front`

For more information about this error, try `rustc --explain E0282`.

EDIT

Ahh, I'm an idiot. I forgot to dump T.

[chanced]

@asmello thanks for the help & review.