InputUsername / listenbrainz-rs

ListenBrainz API bindings for Rust
https://crates.io/crates/listenbrainz
MIT License
11 stars 6 forks source link

Use generics on raw request structs #14

Closed mgziminsky closed 1 year ago

mgziminsky commented 1 year ago

Changes all &str fields in the request module structs to use a generic StrType which is defined as:

pub trait StrType: Borrow<str> + Serialize {}
impl<T: Borrow<str> + Serialize> StrType for T {}

Using &str for the TrackMetadata and other request structs field types makes working with the raw api difficult since it prevents the usage of owned Strings. Using a generic allows much more flexible client usage since this library only needs the type to be serializable. The additional Borrow<str> constraint is added for convenience and usablity.

The wrapper interface is unchanged and continues to use &str.

The motivation for this was a CLI import tool I made recently where I am mapping deserialized structs into Payloads that own their data.

shymega commented 1 year ago

I'm not familiar with Generics in Rust, only Java really, so I can't give much input here. It would make this yet another breaking change, but I did have something I wanted to modify in the library, which would also be a breaking change.

@InputUsername if you want to look over this, a quick glance for me makes it look OK, but it's up to you if you want to use generics.

InputUsername commented 1 year ago

Thanks for the ping @shymega.

I wanted to modify in the library, which would also be a breaking change.

Feel free to open an issue/PR :smile:

shymega commented 1 year ago

For future, probably would have been better to make multiple separate commits, wait for review, then squash into single commit, and merge into mainline.

InputUsername commented 1 year ago

For future, probably would have been better to make multiple separate commits, wait for review, then squash into single commit, and merge into mainline.

I personally don't mind non-squashed commits, but yeah fair enough, probably better indeed :smile:

mgziminsky commented 1 year ago

Github does give the option to perform a squash merge when accepting a PR if that is your preference

shymega commented 1 year ago

Sure, but making multiple commits allows for inspection, then we can squash the commits and merge. What we could also do is push to @mgziminsky's branch. The reason I suggest this is derived from my experiences with Input Leap, where we might make multiple force-pushes to a PR to edit and rebase, then to keep a tidier commit history, we squash it all, and then merge.

shymega commented 1 year ago

If this PR was more long-running, then I suppose it'd make sense. Just my two cents as a maintainer.

InputUsername commented 1 year ago

@mgziminsky I've just released version 0.7.0 with your changes!