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

Newtypes for IDs #84

Closed
rocallahan opened this issue Apr 23, 2021 · 8 comments · Fixed by #85
Closed

Newtypes for IDs #84

rocallahan opened this issue Apr 23, 2021 · 8 comments · Fixed by #85

Comments

@rocallahan
Copy link
Contributor

Seems to me it would be nice to have newtypes for the different IDs (orgs, users, check runs, workflow runs, etc). That would be an API break though. How do you feel about that?

@XAMPPRocky
Copy link
Owner

Thank you for your issue! Overall I'm supportive of the idea. At the very least it can make it easier to ensure all ids are using the correct type (it's currently inconsistent). In terms of implementation I think my preference would be for a macro_rules macro which creates the types for each of the structs, so that they can all share the same methods, while still being distinct types.

@rocallahan
Copy link
Contributor Author

What methods do the newtypes need though? #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)] and maybe Ord/PartialOrd is all I can think of.

@XAMPPRocky
Copy link
Owner

XAMPPRocky commented Apr 23, 2021

Well they'll need ways to extract and map the raw IDs, I don't think the type should be completely opaque, so I think we should have methods and trait impls such as into_inner and Deref/DerefMut implementations, and fmt::Display implementations, as they should still print the numbers.

@rocallahan
Copy link
Contributor Author

I would just make the inner number pub, but ok...

@XAMPPRocky
Copy link
Owner

I mean would also do that. I think it's important for it to be flexible as there are different cases where one style is more preferable than the other.

@rocallahan
Copy link
Contributor Author

I see you've made ids be mostly i64. Do you know of any ids that can actually be negative?

@rocallahan
Copy link
Contributor Author

Another question: what is parse_u64 for? It doesn't do anything usefully different AFAICT

@XAMPPRocky
Copy link
Owner

If there was a reason, I have since forgot it, so feel free to disregard the current setup and make everything u64 for now. If it turns out to be needed we can change that part later.

XAMPPRocky pushed a commit that referenced this issue Apr 30, 2021
* Use newtypes for Github API IDs

Resolves #84

* Bump version due to incompatible API change for IDs

* address review comments
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

Successfully merging a pull request may close this issue.

2 participants