[PR #161] [MERGED] Initial id type proposal #279

New issue

Closed

opened 2026-02-27 20:24:04 +03:00 by kerem · 0 comments

kerem commented

2026-02-27 20:24:04 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/ramsayleung/rspotify/pull/161
Author: @kstep
Created: 11/29/2020
Status: ✅ Merged
Merged: 3/7/2021
Merged by: @marioortizmanero

Base: master ← Head: uri-type

📝 Commits (10+)

fcd9ace initial id type proposal
58e4c19 use strip_prefix() (clippy advice)
c3e722e add try_join() and map_try_join()
24a61dc remove itertools
e79e9bb rewrite try join
f0d26bb cleanup
8155bc4 from for iderror
158eb0b fix try_join/map_try_join
59eab11 remove map_try_join
c4d0ef8 get show, shows, episodes methods use &str instead of String and validate id

📊 Changes

14 files changed (+700 additions, -484 deletions)

View changed files

📝 CHANGELOG.md (+12 -3)
📝 examples/album.rs (+2 -1)
📝 examples/track.rs (+2 -1)
📝 examples/tracks.rs (+3 -2)
📝 examples/with_refresh_token.rs (+4 -3)
📝 src/client.rs (+198 -334)
📝 src/model/enums/types.rs (+2 -2)
➕ src/model/idtypes.rs (+301 -0)
📝 src/model/mod.rs (+60 -0)
📝 src/model/offset.rs (+13 -22)
📝 src/model/track.rs (+14 -2)
📝 tests/test_models.rs (+0 -21)
📝 tests/test_with_credential.rs (+26 -24)
📝 tests/test_with_oauth.rs (+63 -69)

📄 Description

Description

This is a refactoring/change proposal. Introduces several changes and refactorings to the API:

new Id type to represent Spotify id/URI,
a new ClientError variant: InvalidId(IdError),
removed .get_id() and .get_uri() methods,
introduced new dependency: itertools, used it in API methods,
added AsRefStr for Type.

The main point of the change is to introduce Id type. This type abstracts out the following Spotify id/URI operations:

Spotify id/URI parsing and validation,
Spotify URI formatting,
zero-copy: keeps just a slice of original data,
throws an IdError error if id/URI is invalid.

This type replaces both .get_id() and .get_uri() methods.

Also, Spotify methods are changed to use this type instead of .get_id() and .get_uri().
This type, being zero-copy, should also reduce the number of allocations.

This change is a proposal only!

Open Questions/TODO

I'm not sure where should I put the Id and IdError types, put them into model/mod.rs for now.
I'm not sure about tests for Id placement, so I left them in model/mod.rs for now.
Maybe more test-cases should be added.
Maybe it's better to use Cow<'_, str> and Into<Cow<'_, str>> types for internal Id data representation and method arguments.

Motivation and Context

As per TODO comments in code, and as a part of the refactoring effort as per #127 and #160, I decided to propose a new type to abstract out Spotify id/URI operations. Overall improvements:

better input data validation (ids, URIs): invalid ids and URIs now produce errors,
better type-safe Spotify ids representation,
fewer allocations during Spotify ids parsing and processing.

Type of change

Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

I updated tests related to .get_id()/.get_uri() methods.
I ran all the enabled tests using default configuration (with my own credentials in .env) with cargo test command.

This change is

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/ramsayleung/rspotify/pull/161 **Author:** [@kstep](https://github.com/kstep) **Created:** 11/29/2020 **Status:** ✅ Merged **Merged:** 3/7/2021 **Merged by:** [@marioortizmanero](https://github.com/marioortizmanero) **Base:** `master` ← **Head:** `uri-type` --- ### 📝 Commits (10+) - [`fcd9ace`](https://github.com/ramsayleung/rspotify/commit/fcd9ace324c373513ba9ff157081608ca644e3a1) initial id type proposal - [`58e4c19`](https://github.com/ramsayleung/rspotify/commit/58e4c19c79d14140639263636fc0e45aa91b7c7a) use strip_prefix() (clippy advice) - [`c3e722e`](https://github.com/ramsayleung/rspotify/commit/c3e722e9e349c9d7b3fbe108f5c2b17d97ad8ae6) add try_join() and map_try_join() - [`24a61dc`](https://github.com/ramsayleung/rspotify/commit/24a61dc023a65a53772c4a9f85656b45baac3ab3) remove itertools - [`e79e9bb`](https://github.com/ramsayleung/rspotify/commit/e79e9bb2f1af6f973fdc22d1994c3ffe20681f96) rewrite try join - [`f0d26bb`](https://github.com/ramsayleung/rspotify/commit/f0d26bb6a640f55999aa54b791768e58efeeec1a) cleanup - [`8155bc4`](https://github.com/ramsayleung/rspotify/commit/8155bc40bd98f70f7b03442b5affb22634c45181) from for iderror - [`158eb0b`](https://github.com/ramsayleung/rspotify/commit/158eb0b2796cd7c9291d1cac4710716395977a4d) fix try_join/map_try_join - [`59eab11`](https://github.com/ramsayleung/rspotify/commit/59eab11e6fcf37960e65088e8cde3a0b98a8edc1) remove map_try_join - [`c4d0ef8`](https://github.com/ramsayleung/rspotify/commit/c4d0ef80f473fab2f7b97f18bc1ec9c88d35f679) get show, shows, episodes methods use &str instead of String and validate id ### 📊 Changes **14 files changed** (+700 additions, -484 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+12 -3) 📝 `examples/album.rs` (+2 -1) 📝 `examples/track.rs` (+2 -1) 📝 `examples/tracks.rs` (+3 -2) 📝 `examples/with_refresh_token.rs` (+4 -3) 📝 `src/client.rs` (+198 -334) 📝 `src/model/enums/types.rs` (+2 -2) ➕ `src/model/idtypes.rs` (+301 -0) 📝 `src/model/mod.rs` (+60 -0) 📝 `src/model/offset.rs` (+13 -22) 📝 `src/model/track.rs` (+14 -2) 📝 `tests/test_models.rs` (+0 -21) 📝 `tests/test_with_credential.rs` (+26 -24) 📝 `tests/test_with_oauth.rs` (+63 -69) </details> ### 📄 Description ## Description This is a refactoring/change proposal. Introduces several changes and refactorings to the API: - new `Id` type to represent Spotify id/URI, - a new `ClientError` variant: `InvalidId(IdError)`, - removed `.get_id()` and `.get_uri()` methods, - introduced new dependency: `itertools`, used it in API methods, - added `AsRefStr` for `Type`. The main point of the change is to introduce `Id` type. This type abstracts out the following Spotify id/URI operations: - Spotify id/URI parsing and validation, - Spotify URI formatting, - zero-copy: keeps just a slice of original data, - throws an `IdError` error if id/URI is invalid. This type replaces both `.get_id()` and `.get_uri()` methods. Also, `Spotify` methods are changed to use this type instead of `.get_id()` and `.get_uri()`. This type, being zero-copy, should also reduce the number of allocations. This change is a proposal only! ## Open Questions/TODO - I'm not sure where should I put the `Id` and `IdError` types, put them into `model/mod.rs` for now. - I'm not sure about tests for `Id` placement, so I left them in `model/mod.rs` for now. - Maybe more test-cases should be added. - Maybe it's better to use `Cow<'_, str>` and `Into<Cow<'_, str>>` types for internal `Id` data representation and method arguments. ## Motivation and Context As per TODO comments in code, and as a part of the refactoring effort as per #127 and #160, I decided to propose a new type to abstract out Spotify id/URI operations. Overall improvements: - better input data validation (ids, URIs): invalid ids and URIs now produce errors, - better type-safe Spotify ids representation, - fewer allocations during Spotify ids parsing and processing. ## Type of change - Breaking change (fix or feature that would cause existing functionality to not work as expected) - This change requires a documentation update ## How Has This Been Tested? - I updated tests related to `.get_id()`/`.get_uri()` methods. - I ran all the enabled tests using default configuration (with my own credentials in `.env`) with `cargo test` command.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/ramsayleung/rspotify/161)  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>