A great deal of functions have inconsistent argument types

XAMPPRocky / octocrab

A modern, extensible GitHub API Client for Rust.

Other

1.06k stars 258 forks source link

A great deal of functions have inconsistent argument types #568

Open manchicken opened 7 months ago

manchicken commented 7 months ago

There's very little in the way of consistency of type in function arguments.

For example, src/api/issues.rs has a couple of functions which are similar in function and form, but have incompatible types requiring different calling conventions.

L606 - pub async fn delete_comment(&self, comment_id: CommentId) -> Result<()>
L1079 - pub async fn delete_comment_reaction(&self, comment_id: impl Into<CommentId>, reaction_id: impl Into<ReactionId>) -> Result<()>

Note that in one of the functions, comment_id is CommentId, and in the other it's impl Into<CommentId>.

Fixing this will require breaking changes.

XAMPPRocky commented 7 months ago

Yeah, this is what happens when you have a large amount of contributors 😄 Should add the style and such to the CONTRIBUTING.md.

impl Into (or impl AsRef where appropriate) should generally be preferred since it removes boilerplate from the user.

Also not sure that changing to impl Into is considered a breaking change. Technically it does change the function signature, but there's nothing that works with fn (CommentId) that won't also work with fn (impl Into<CommentId>).

manchicken commented 7 months ago

Oh yeah, many chefs and such. I don't think this is a huge problem, but it's something that takes the polish off. It's exciting to have so much interesting stuff to do with this library.

I think it will break some code; I tried making this change got delete_comment() last night, and when I did it broke tests.