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

adding code_with_match (#1024) #1031

Merged
merged 4 commits into from
Oct 3, 2021
Merged

adding code_with_match (#1024) #1031

merged 4 commits into from
Oct 3, 2021

Conversation

QuentinRa
Copy link
Contributor

Hi! As described in #1024, I added a function code_with_match.

Copy link
Collaborator

@acrobat acrobat left a 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

/**
* @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'));
}

*
* @return array list of code found
*/
public function code_with_match($q, $sort = 'updated', $order = 'desc')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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');
Copy link
Collaborator

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

@akehsanz

This comment has been minimized.

@QuentinRa
Copy link
Contributor Author

QuentinRa commented Oct 3, 2021

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).

Copy link
Collaborator

@acrobat acrobat left a 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!

Comment on lines 70 to 72
* @param $q the filter
* @param $sort the sort field
* @param $order asc/desc
Copy link
Collaborator

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

@acrobat acrobat merged commit fe55989 into KnpLabs:master Oct 3, 2021
@acrobat
Copy link
Collaborator

acrobat commented Oct 3, 2021

Thanks @QuentinRa! And congrats on your first contribution! 🎉

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.

None yet

3 participants