Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A great deal of functions have inconsistent argument types #568

Open
manchicken opened this issue Feb 8, 2024 · 2 comments
Open

A great deal of functions have inconsistent argument types #568

manchicken opened this issue Feb 8, 2024 · 2 comments

Comments

@manchicken
Copy link
Sponsor Contributor

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
Copy link
Owner

XAMPPRocky commented Feb 9, 2024

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
Copy link
Sponsor Contributor Author

manchicken commented Feb 9, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants