search

octokit / octokit.rb

Ruby toolkit for the GitHub API
http://octokit.github.io/octokit.rb/
MIT License
3.84k stars 1.13k forks source link

[BUG]: Fetch a given reference uses the wrong API #1628

Open kitallis opened 1 year ago

kitallis commented 1 year ago

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) not ref (singular) which causes this API to behave like the matching-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

github-actions[bot] commented 1 year ago

👋 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 Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

nickfloyd commented 1 year ago

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!

kitallis commented 1 year ago

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 ref is a required field. While simultaneously saying the bit about When you use this endpoint without providing a :ref... etc. Which means it's technically not a required field?

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.

nickfloyd commented 1 year ago

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 ❤️ !

kitallis commented 7 months ago

This is still on my radar. I will come back to it soon.