InputUsername / listenbrainz-rs

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

Split up long modules (raw client & request/response models) #25

Open InputUsername opened 7 months ago

InputUsername commented 7 months ago

Some modules are getting really big (500+ lines), so we could split them up:

The ListenBrainz API has been subdivided into several categories, so we could split based on those:

The client might be split up by implementing the different categories as traits, not sure how well that would work though.

RustyNova016 commented 7 months ago

Seems fine for me. might as well split up by sub category but might be too much?

As for the client, maybe just do several impl blocks?

InputUsername commented 7 months ago

Seems fine for me. might as well split up by sub category but might be too much?

Can always split up later if needed but I think just by category is fine for now.

As for the client, maybe just do several impl blocks?

I think having multiple impl blocks across files isn't idiomatic for Rust, but not sure what the right approach is.

RustyNova016 commented 7 months ago

From what I could gather on this, the principal feeling is to first try to deleguate fuctions to other structs (which is useless here), create traits if they have meaningful separations (Aka, that trait is useful for other structs too), or you can split impl accross modules, but this come with a little trick if you are using private fields.

InputUsername commented 7 months ago

From what I could gather on this, the principal feeling is to first try to deleguate fuctions to other structs (which is useless here), create traits if they have meaningful separations (Aka, that trait is useful for other structs too), or you can split impl accross modules, but this come with a little trick if you are using private fields.

Seems good, so essentially you'd have e.g. raw/client.rs defining struct Client and then raw/client/core.rs, raw/client/playlists.rs with the impl block per category. :+1:

Same for raw/response.rs (shared types/traits) and raw/response/{core,playlists,...}.rs per category.

Not sure about raw/request.rs since it's not that big yet (<100 lines), but might as well.

RustyNova016 commented 7 months ago

raw/request.rs can be done later, but I'd say it's better to do it at one point for consistency. But that structure is fine IMO

One thing I'm wondering about, is where can we put methods for raw responses. Some raw responses could benifit from have having little abstraction methods or getters to fetch deeply nested data (for exemple, a method to tell whether a listen is linked, or one to get the listen recording name that is explicitly stating that it isn't the mapping's record name).

Either we need to put them in the same file, or make raw/response/{core,playlists,...}.rs into raw/response/{core,playlists,...}/mod.rs, allowing us to add an impl.rs for implements.