-
-
Notifications
You must be signed in to change notification settings - Fork 599
adding code_with_match (#1024) #1031
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
Conversation
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 @QuentinRa! Few minor comments and can you add a test for this method? See the related code search for a test example
php-github-api/test/Github/Tests/Api/SearchTest.php
Lines 93 to 111 in de92500
/** | |
* @test | |
*/ | |
public function shouldSearchCodeByQuery() | |
{ | |
$expectedArray = [['total_count' => '0']]; | |
$api = $this->getApiMock(); | |
$api->expects($this->once()) | |
->method('get') | |
->with( | |
'/search/code', | |
['q' => 'query text', 'sort' => 'updated', 'order' => 'desc'] | |
) | |
->will($this->returnValue($expectedArray)); | |
$this->assertEquals($expectedArray, $api->code('query text')); | |
} |
lib/Github/Api/Search.php
Outdated
* | ||
* @return array list of code found | ||
*/ | ||
public function code_with_match($q, $sort = 'updated', $order = 'desc') |
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.
public function code_with_match($q, $sort = 'updated', $order = 'desc') | |
public function codeWithMatch(string $q, string $sort = 'updated', string $order = 'desc') |
Also remove the redundant phpdoc after this change
* @param string $q the filter
* @param string $sort the sort field
* @param string $order asc/desc
doc/search.md
Outdated
```php | ||
$files = $client->api('search')->code('@todo language:php'); | ||
``` | ||
|
||
Returns a list of files found by such criteria (containing "@todo" and language==php). | ||
|
||
```php | ||
$files = $client->api('search')->code_with_match('@todo language:php'); |
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.
Update the method after the change below
This comment has been minimized.
This comment has been minimized.
Thanks! I looked for tests, and for CONTRIBUTING.md/code_style.*, but I haven't found any (I should've checked by myself, instead of relying on my IDE 😖). I added the return type too. I'm not sure if I should remove the "redundant" types in the DOC, my IDE is showing a warning 😶"Update PHPDoc comment - Argument type do not match the declared type" (PHPStorm). I'm not sure if my tests are what you wanted, I can't run them at all (and I don't have much time to look into it). |
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.
I comment left based on the failing phpstan run. Otherwise the PR is good to go!
lib/Github/Api/Search.php
Outdated
* @param $q the filter | ||
* @param $sort the sort field | ||
* @param $order asc/desc |
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.
Please remove these phpdocs as the are redundant with the added typehint's
Thanks @QuentinRa! And congrats on your first contribution! 🎉 |
Hi! As described in #1024, I added a function
code_with_match
.