Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 16, 2024

Conversation

asmello
Copy link
Collaborator

@asmello asmello commented Jun 15, 2024

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
Copy link
Collaborator Author

asmello commented Jun 15, 2024

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
Copy link
Owner

chanced commented Jun 16, 2024

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
Copy link
Owner

chanced commented Jun 16, 2024

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.

@chanced chanced merged commit a54c707 into chanced:main Jun 16, 2024
1 check passed
@asmello
Copy link
Collaborator Author

asmello commented Jun 16, 2024

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
Copy link
Owner

chanced commented Jun 17, 2024

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
Copy link
Collaborator Author

asmello commented Jun 17, 2024

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

@asmello asmello mentioned this pull request Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Token::as_index OOB behavior
2 participants