chanced / jsonptr

JSON Pointer (RFC 6901) implementation for Rust
Apache License 2.0
44 stars 4 forks source link

introduce Index type for less error-prone index resolution #39

Closed asmello closed 2 months ago

asmello commented 2 months ago

Fixes #35

As discussed in #35, this PR introduces a new type Index which encodes a parsed index from a Token type (or more generally, an RFC 6901-compliant string).

This type is primarily useful to resolve the - token into a concrete numerical index given a known array length. It's also useful for reliably checking bounds against the provided length.

asmello commented 2 months ago

I'm on the fence now about just using for_len as the name for the validating exclusive method. It reads nicely:

let num = token.to_index()?.for_len(42)?;

Even more so in Index::Next.for_len(42), but I doubt that will come up in real code.

Then we could rename the other methods for_len_incl (or for_len_next) and for_len_unchecked.

chanced commented 2 months ago

Ugh, I'm sorry I was slow on the draw on this one. Between getting my daughter a pup and fathers day, I definitely slacked this weekend.

I'm going kayaking with the family today so I won't be available to run through it yet.

I may just hook into this PR and refactor the errors here. It is the last major thing that I want to change about the crate. Dealing with the errors that currently exist is somewhat obnoxious.

chanced commented 2 months ago

Nevermind, I'm just going to create a new PR.

Thank you for all of your hard work! I'm sorry the code committed since the start of your participation has been so one sided.

Getting this released will be my top priority going into the week. We should be able to cut 0.5 on monday.

asmello commented 2 months ago

Ugh, I'm sorry I was slow on the draw on this one. Between getting my daughter a pup and fathers day, I definitely slacked this weekend.

I'm going kayaking with the family today so I won't be available to run through it yet.

Don't worry, man. Your family takes priority by a long shot. While I really appreciate your responsiveness, there's no time pressure here, so please pace yourself. Maintaining an open-source project is always a lot of work. I'm glad you're having fun with your daughter and family, I'm a bit jealous of the kayaking.

Thank you for all of your hard work! I'm sorry the code committed since the start of your participation has been so one sided.

It's cool, I'm just doing these changes because I'm in the mood for it and I happen to have enough time for it right now. We're both volunteers here, you facilitating these changes is more than enough.

By the way, thanks a lot for merging, but did you see my comment about for_len? I'm half convinced that's a better naming choice. Happy to push a follow-up PR if you agree.

chanced commented 2 months ago

You’re awesome dude. Seriously.

No, I missed the comment. I’ll be honest, your skillset is well above mine so it takes some time for me to process/understand it all. I’ve been accepting them so not to hold it up and then analyzing it post-merge. I trust that you aren’t malicious and I doubt id catch anything subtle until I'm more well versed in dealing with dynamically sized types.

Re: kayaking. Wife, daughter and I picked it up over COVID and absolutely love it. Truly my favorite activity. You should grab yourself a cheap, used sit-on and see if you enjoy it. Gets you out in nature and it's pretty good exercise.

Edit: actually, I forgot my wife and I hired a guide first before we dove into it. That's probably the way to go, so you can see if it's your cup of tea first.

asmello commented 2 months ago

No, I missed the comment. I’ll be honest, your skillset is well above mine so it takes some time for me to process/understand it all. I’ve been accepting them so not to hold it up and then analyzing it post-merge. I trust that you aren’t malicious and I doubt id catch anything subtle until I'm more well versed in dealing with dynamically sized types.

Gotcha. Since you haven't disagreed with the names I mentioned I'll open another PR then. ;)