Add overloads to specify branch for ContentRequest

[M-Zuber]

Fixes #958

[ryangribble]

Typo in the xml doc with the cref to the class name

[ryangribble]

Hey @M-Zuber
This should be an awesome function, a tool we are writing at the moment has a need to push a file directly into a specific branch too 😀

I noticed one typo of a class name in XmlDoc comments, ive flagged above.

Also could I suggest ensuring we keep the tests up to date with at least these 2 items?

• update the unit tests to cover off the addition of the new branch parameter
• add an integration test to prove it actually creates the file on the specified branch

Let me know if you want some help adding the tests

[M-Zuber]

Thank you for catching the typo!
I should be able to add unit tests after the weekend.

Would you test anything in the unit tests other then that the correct url was called?

The integration test can create, update, and then delete a file on the same branch. Sounds good to you?

[ryangribble]

Yeah if you look at the "crud" integration test I'd probably do the same actions as that, but specifying a named branch

For unit test since the url is the same anyway, you could check that the CreateFileRequest object passed through, has the correct path/message/content/branch present in it. Eg have look at PassesRequestObject() test here

[M-Zuber]

I wasn't able to get the unit tests to work, hopefully the integration test will be enough

[ryangribble]

Hey @M-Zuber that looks good... your integration test is passing for me here.

On the topic of unit tests, your change was mostly to a Request object and it doesnt look like there are many unit tests of those things in octokit.net.

However if you are interested, I did just send you a pull request that implements unit tests on the CreateFile() DeleteFile() and UpdateFile() calls in RepositoryClient, including making sure your new branch field is correctly passed through to the underlying ApiClient. What ive found with Octokit unit tests for API clients typically check

• calls the correct URL
• passes through the request object
• checks parameters for nulls and throws exceptions if paramter is required

M-Zuber#2

[khellang]

I don't think this state machine is necessary. Assert.ThrowsAsync expects a Task, which GetAllContents already returns 😄

[M-Zuber]

Copy paste error 😄
Will fix when I'm back by a machine.

It is needed. If removed the test completes, but fails with a message that no error was thrown.
EDIT: Drunk VS had me today/

[M-Zuber]

@ryangribble thank you for the PR. I asked a question there.

[Thwaitesy]

@ryangribble when do you think this will be merged?

[ryangribble]

I don't have commit rights, am sitting on a few pending PR's myself 😀

[shiftkey]

@Thwaitesy @ryangribble apologies for the delay, I'm aiming to get through these by the end of the week. Thankfully we just did a big release, so the next one should be a bit easier to manage...

[shiftkey]

Code looks good, tests all pass, and it got a pretty great review too...

[M-Zuber]

YAY!! I got a Stevie!! 😀