Skip to content

Conversation

@wfraser
Copy link
Contributor

@wfraser wfraser commented Nov 12, 2025

For both of these, the only error case is a non-utf8 string, which is impossible since we're building it from a fixed alphabet.

This allows removing a bunch of unnecessary error handling branches.

Also, for base16, there's no need to implement parsing/formatting, just use the stdlib's hex conversion/formatting code.

The only error case is a non-utf8 string, which is impossible since we're building it from a fixed alphabet.

This allows removing a bunch of unnecessary error handling branches.
Copy link
Member

@photovoltex photovoltex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to asked about the write!().unwrap() call in file_id.rs, but as it turns out some searching and understanding gives the answer.

fmt::Write for String never returns an error itself

Seems like a reasonable change. Please add a changelog entry to give this the final touch, then we could probably merge this.

@wfraser wfraser requested a review from photovoltex November 12, 2025 21:51
@wfraser wfraser force-pushed the spotify-id-base62-infallible branch from 361a09d to f2e80f0 Compare November 13, 2025 19:23
@wfraser wfraser requested a review from photovoltex November 13, 2025 19:24
@photovoltex photovoltex merged commit f3a3463 into librespot-org:dev Nov 14, 2025
12 checks passed
@photovoltex
Copy link
Member

Thanks for the change :)

@photovoltex
Copy link
Member

Just found out that to_uri could have been made infallible. Did miss that in the review.

@wfraser
Copy link
Contributor Author

wfraser commented Nov 14, 2025

Oh, nice find, I can do a quick PR for that too.

@photovoltex
Copy link
Member

If you want to, go for it :)

photovoltex pushed a commit that referenced this pull request Nov 15, 2025
@wfraser wfraser deleted the spotify-id-base62-infallible branch November 17, 2025 18:47
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.

2 participants