-
Notifications
You must be signed in to change notification settings - Fork 75
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
Fetch issue comments #188
Fetch issue comments #188
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.
Looks great 👍 , I just a couple of comments. Did you find the contributing guide useful?
docs/src/main/tut/issue.md
Outdated
- the repository coordinates (`owner` and `name` of the repository). | ||
- `number`: The issue number. | ||
|
||
To create a comment: |
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.
To list comments:
, no?
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.
yes, you are right
issues.listComments(None, headerUserAgent, validRepoOwner, validRepoName, validIssueNumber) | ||
response should be('left) | ||
} | ||
|
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 think you're missing the mocked responses: http://47deg.github.io/github4s/contributing.html#mocking-the-responses
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.
also true
@BenFradet yes, it is helpful. I got a good overview of what has to be done. |
10f5140
to
e05fa05
Compare
@BenFradet fixed btw - sry for the late ping |
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.
LGTM 👍 , putting this on hold until we fix travis in #190
@GRBurst Could you rebase against master? |
e05fa05
to
2d7bb43
Compare
2d7bb43
to
c5e18ac
Compare
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.
Tested locally, works fine
Thanks again @GRBurst
Thanks @fedefernandez , merging 👍 |
Thanks @BenFradet for taking care |
Hey,
fixes #186. Already used it successfully in my fork.
Fits contributing guidelines I think.
Kind regards