-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
Add Support For GitData Reference Matching Methods #875
Add Support For GitData Reference Matching Methods #875
Conversation
Added two new methods to GitData->References to cover the matching reference endpoints described here: https://developer.github.com/v3/git/refs/#list-matching-references
Add New Tests In ReferencesTest For Our Two New Methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @nickpoulos! I have just a few small comments.
- Add rawurlencode around both the $branch and $tag parameters in matchingBranch() and matchingTag() respectively. - Add parameter type hints, and function return types for both marchingBranch() and matchingTag()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @nickpoulos! One thing I forgot to mention, can you add a docs entry for the newly added endpoints?
After that it's good to be merged!
Sure thing, let me write something up I'll get it in there later tonight. Thanks man |
Hmm, after writing the docs, I am wondering if I am not quite sticking to conventions the package has set. For example to show a reference, based on a tag or branch, you must include the either 'head/' or 'tag/' as part of the last argument, like so: Show a reference$reference = $client->api('gitData')->references()->show('KnpLabs', 'php-github-api', 'heads/featureA'); The last argument contains both '/head/'and the branch name, and would contain 'tag/' and a tag name for tags, so they can use one endpoint and method for both tag/branch. I have abstracted that out into two methods, where you don't prefix 'head/' or 'tag/' in your third argument. I am suggesting I rewrite to a singular method as the rest of the package appears to do. |
Add Documentation For method matching() in gitdata/references.md
- In order to match existing API conventions, the two method solution for branch/tag was consolidated into one method and extra data in the third argument. - Updated ReferencesTest.php to only test the single method
Remove link from DocBlock
Can you also fix the styleci issue? (I know it's a nitpick but this will let all pr checks pass before I merge 😄) |
- reference parameters for the API can contain a '/', luckily we already have a method to handle these cases
Yea I noticed this, and I would love to fix it. But when I click on the StyleCI details it gives me a very generic error:
And then I can't seem to find any more information. When I examine the code it pointed out as incorrect, nothing glaring jumps out at me? Any ideas there? Sorry if this is another easy one, my StyleCI-foo is prob not up to snuff. |
@@ -25,6 +25,22 @@ public function all($username, $repository) | |||
return $this->get('/repos/'.rawurlencode($username).'/'.rawurlencode($repository).'/git/refs'); | |||
} | |||
|
|||
/** | |||
* Get all matching references for the supplied reference name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickpoulos this is the line where styleci "complains" about. It should end with a "."
Update ReferencesTest.php To Use A More Realistic Parameter
StyleCI Add Period
Thanks @nickpoulos! And congrats on your first contribution! 🎉 |
Thank you! Happy to be able to help for once! |
This PR adds two methods -
matchingBranch()
andmatchingTag()
toGitHub/Api/GitData/References.php
. These methods return the references for a given branch or tag, respectively.There are two more tests to
GitHub/Tests/Api/GitData/ReferencesTest.php
,shouldGetAllMatchingBranchRepoReferences
andshouldGetAllMatchingTagRepoReferences
.Documentation here: https://developer.github.com/v3/git/refs/#list-matching-references