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

Allow fully qualified or short references to be used to generate "reference" Uri #1934

Closed
wants to merge 0 commits into from

Conversation

astrohart
Copy link
Contributor

@astrohart astrohart commented Jan 30, 2019

Fixes #1933

30 Jan 2019 Updated the second overload of the ApiUrls.Reference to remove any occurrence of refs/ from the string passed in for its referenceName parameter, so a reference from a tag retrievd with the API can be used natively with other reference APIs, such as Delete Reference.

@ryangribble
Copy link
Contributor

Any reason why you've removed the .dotsettings resharper settings file @astrohart ?

@ryangribble
Copy link
Contributor

ryangribble commented Jan 31, 2019

I'm ok with this change - I agree it is robust and handles both the "existing" behaviour where refs/ wasnt specified in the argument, plus works with "new" behaviour where you quite rightly point out, a ref received from another endpoint should be able to be passed straight into the delete method and work.

It actually stems from the GitHub Api documentation itself, where it indicates the URI for patch and delete calls is /repos/:owner/:repo/git/refs/:ref (ie :ref should not include prefix of refs/) yet on the create call requires the "fully qualified ref" to be specified:

ref | string | Required. The name of the fully qualified reference (ie: refs/heads/master). If it doesn't start with 'refs' and have at least two slashes, it will be rejected.

Nevertheless I think this change makes things less surprising and better for users so I'm cool with it...

I would ask 2 things though:

  • if we no longer require the ref to be formatted in a particular way, could we remove that XmlComment in the code, that tells users not to pass in refs/ ?
  • Since the Update (patch) endpoint has the same behaviour, can we ensure this is also updated/tested in octokit with similar changes? (i am on mobile so haven't checked if it uses the same ApiUrl overload and/or has the same comment guidance about not passing refs/ in)

@ryangribble ryangribble changed the title Fixed #1933 Allow fully qualified reference to be passed into DeleteReference method (Fixes #1933) Jan 31, 2019
@ryangribble ryangribble changed the title Allow fully qualified reference to be passed into DeleteReference method (Fixes #1933) Allow fully qualified or short references to be used to generate "reference" Uri (Fixes #1933) Jan 31, 2019
@shiftkey shiftkey changed the title Allow fully qualified or short references to be used to generate "reference" Uri (Fixes #1933) Allow fully qualified or short references to be used to generate "reference" Uri Feb 24, 2020
@shiftkey shiftkey closed this Feb 24, 2020
@shiftkey
Copy link
Member

I accidentally closed this out because I could rewrite the branch to update the history and drop the tooling commit. I've opened #2104 to restart the process and see if we can land the fix.

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 this pull request may close these issues.

Octokit says that a tag does not exist when it does, in fact
3 participants