-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[BUG]: Fetch a given reference uses the wrong API #1628
Comments
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with |
Hey @kitallis thank you for bringing this up. This particular API set is confusing. So we have all of the GitHub REST API set for references - breaking them down below quickly for clarity - It looks like we have a couple of problems. All this to say, thank you for being open to get this sorted, please feel free to do so! ❤️ Also, even though this is wrong I'm feeling like it will be a breaking change - we'll version as such so that anyone using these SDKs will have a heads up on the change. Here are the details. Please feel free to correct both implementations if you have time. Head's up on the cassette, they might need to be regenerated.
Thank you again for helping to make our community better! |
Thanks @nickfloyd. I had to squint to understand why you pointed out the "List matching refs" to be an incorrect implementation. Just adding some more info here for clarity: List matching refs is actually just one API. Documented here. This API also claims in the docs, that the This perhaps means that the octokit interface should either optionally take a ref or implement the two ways as different methods. I will give a shot at "Get a reference" over the weekend since that's my current need. If the others are equally simple to change, I can correct them as well. |
Hey @kitallis thanks for the follow up here. I'll leave the implementation in your capable hands! Either implement as an optional parameter or as an overload with different signatures (as two separate methods). My gut would favor being more explicit and having two methods but SDKs are meant to abstract out complexity and expressiveness so that consumers don't have to know the implementation details of a given API - in this case I feel like both options that you present still serve the SDK philosophy well. I'm looking forward to seeing what you come up with! As always thank you for being a member of this community and working to make things better for everyone ❤️ ! |
This is still on my radar. I will come back to it soon. |
What happened?
The following method: https://github.com/octokit/octokit.rb/blob/main/lib/octokit/client/refs.rb#L46 is presumably the analog to this API.
However, the endpoint used is
refs
(plural) notref
(singular) which causes this API to behave like thematching-refs
API, which I think is incorrect since we already have a separate API for that.We can check that the API does produce unintended output, from this example: https://api.github.com/repos/tramlinehq/ueno/git/refs/tags/v10 (which is what the method uses).
In here,
refs/tags/v10
is not a valid ref in this case.If this issue seems genuine, happy to drop a PR to correct it.
Versions
Octokit.rb v7.1.0
Relevant log output
No response
Code of Conduct
The text was updated successfully, but these errors were encountered: