XAMPPRocky/octocrab

A great deal of functions have inconsistent argument types

Opened this issue · 2 comments

manchicken commented

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

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

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.

  • 😄1