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

Adds list and search methods for issues #47

Merged
merged 2 commits into from
Jan 4, 2017

Conversation

fedefernandez
Copy link
Contributor

This PR adds the list and search methods for issues

Please @juanpedromoreno could you review? I'm especially interested in your opinion with the definition of SearchParam

Thanks!

Copy link
Member

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

This looks great!!

Minor comments added though.

.execFuture(headerUserAgent)

testFutureIsRight[SearchIssuesResult](response, { r =>
r.result.total_count > 1 shouldBe true
Copy link
Member

Choose a reason for hiding this comment

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

This assert is saying that at least should exist two issues, which is incongruent with the test banner return at least one issue for a valid query


response should be('right)
response.toOption map { r ⇒
r.result.total_count > 1 shouldBe true
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

*
* @param accessToken to identify the authenticated user
* @param headers optional user headers to include in the request
* @param searchParams list of search params
Copy link
Member

Choose a reason for hiding this comment

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

Let's include what query parameter does too, to complete the scaladoc ;)

@codecov-io
Copy link

codecov-io commented Jan 4, 2017

Current coverage is 86.34% (diff: 75.55%)

Merging #47 into master will decrease coverage by 1.65%

@@             master        #47   diff @@
==========================================
  Files            22         27     +5   
  Lines           350        388    +38   
  Methods         348        384    +36   
  Messages          0          0          
  Branches          2          3     +1   
==========================================
+ Hits            308        335    +27   
- Misses           42         53    +11   
  Partials          0          0          

Powered by Codecov. Last update df49ef1...3fc5fed

  * Adds tests for JS
  * Fixes problems with encoding params
@fedefernandez
Copy link
Contributor Author

Ready for a second review!

Thanks @juanpedromoreno

@juanpedromoreno
Copy link
Member

LGTM! Thanks @fedefernandez !

@fedefernandez fedefernandez merged commit f6f79f0 into master Jan 4, 2017
@fedefernandez fedefernandez deleted the search-and-list-issues branch January 4, 2017 13:07
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.

3 participants