Implementing Deployments client #298
Conversation
Just checking that we get something back when creating a deployment and that we can get deployments from a repository that is known to have them.
IDeploymentsClient and IDeploymentStatusesClient and related request/ response classes.
because it's what it should have been all along.
and cleaned up Integration/DeploymentClientTests.cs some.
|
Note to self, forgot to doc comment the ApiUrls. Also need to add remarks on some of the API methods with links to the documentation |
|
@pmacn Just document the interfaces and then run the |
|
I know, I built that. ;) The issue here was that I forgot to comment the ApiUrl methods I added, just a minor thing but it looked as if most the other ones were documented so I figured I should as well. One of these days I'll set up my gravatar to make me a little more recognizable :) |
|
|
|
|
|
And thanks again for that contribution. I'll try hard to remember to associate @pmacn with that. :) In fact, that's one of the things i'm excited to demonstrate on Monday! |
to api documentation for methods.
Not sure it needs a serialization one since the client should never have to serialize them
DeploymentStatus as well. These are technically already covered with the integration tests but this way they can be run without running the integration tests.
|
Awesome! Good luck with the talk/demo, wish I could go but wrong part of the country. I do have some plans to flesh that project out further and maybe have it become an actual useful project of its own. But, time and such. Anyway, I think those added commits has me fairly satisfied with this PR. Unless tomorrow also turns into playing star wars with the kid I should hopefully get the Rx one started/done. But that will be a separate PR. |
|
The reactive clients are completed with the exception of some unit tests which are at the moment proving a lot more complex than initially thought, #300 |
|
Was going to rebase this onto the changes that have already been pushed to master but after doing that locally I'm drawing a blank as to whether I can actually get that into the same PR somehow. Or do I have to open up a new PR? |
|
It's your branch at this point, I'm not fussed whether you merge master in or rebase to update it. What you'll see here is the commits appear after the last comment (if rebasing). Up to you. |
|
On second thought, I'm not even sure why I was trying to do that. Especially since there doesn't seem to be any issues with the pull request. I wonder if someone replaced my coffee with decaf. These are clearly ramblings of a madman and should be ignored. |
|
Pushing the reactive clients into this PR as well |
|
|
||
| static readonly string[] whitespaceArguments = { " ", "\t", "\n", "\n\r", " " }; | ||
|
|
||
| public static async void ThrowsWhenGivenWhitespaceArgument(Func<string, Task> action) |
There was a problem hiding this comment.
If you make this method return a Task then you can await on it (like you do with AssertEx.Throws) and the tests will past as part of the CI build script.
|
Yeah, not sure what I was thinking there. Clearly I need to write 100 "only use async void for fire and forget" on the white board. |
|
✨ and this is why we have code reviews 😀 |
|
Seems I also mucked up some stuff when getting the reactive client onto this branch. Calling the wrong method in some tests, etc. If only I'd have been drunk so I had an excuse. Also seems as if testing arguments is going be an issue on the create methods since they just call into the non-reactive client. Which is being Substituted. (We do have a number of other tests in our test suite that seem to be the same way. They're just not awaited so they don't actually do anything. So I could either add the checks in the Create method on the reactive client and the tests will pass but we'll be performing the same Ensure calls twice in actual use. Or we could just be happy that it tests that the non-reactive client is being called. Knowing that it in turn is covered by argument tests. A third option would be to make the substituted GitHubClient return an actual non-reactive client for the reactive client to call into. That way we get our tests on the reactive client so that the requirement is not forgotten in case it's one day decided that it should no longer call into the non-reactive client. |
|
My vote is for the third option. |
Client and ObservableDeploymentStatusClient. Needed to make sure that the tasks were actually run and not just scheduled.
DeploymentStatusClient, ObservableDeploymentClient and ObservableDeploymentStatusClient.
|
The functionality gotten from AssertEx.ThrowsWhenGivenWhitespaceArgument could be had using the assertion methods in #319 instead. |
|
And this should all be fixed now, unless I'm blind and missing something. It's amazing how quickly I go blind when looking at my own code compared to others. |
|
Probably need to merge master into this again. |
| Ensure.ArgumentNotNullOrEmptyString(name, "name"); | ||
|
|
||
| return _connection.GetAndFlattenAllPages<Deployment>(ApiUrls.Deployments(owner, name), null, | ||
| "application/vnd.github.cannonball-preview+json"); |
There was a problem hiding this comment.
Hey buddy. What are you doing way over there on the right. Come over here. It's ok, we won't bite. Perhaps one tab in like you do elsewhere. 😄
There was a problem hiding this comment.
My own conventions sneaking in there. I generally line up arguments either all on new lines or lined up with the first argument. Dot-notation indentation is a whole other story.
|
Looking pretty good to me. Just need you to light up that green button so I can merge it. 😄 |
…deployments_client
|
Merged in master and fixed the whitespace issues |
Implementing Deployments client
|
|
Apparently the # references don't work in titles. so putting it here. Issue #297
I'm having some doubts about a few naming things still, especially some inconsistencies regarding status/statuses. Just something to keep an eye out for and I wouldn't mind a comment or two on that if someone has an opinion about it.
There is also a huge chunk of duplicated code between the two integration tests classes, not to mention that a lot of that came from an already existing integration test. It's still DRY if I'm repeating someone else's code, right?
Anyway, I'm sure I could put some effort into cleaning that up and maybe implement some sort of setup "builder" if that's something we'd want to do. That could hopefully replace the setup in the other integration tests as well, making them all more declarative.
Other than that I hope that this should be satisfactory for the most part; After all it was a rather straight forward implementation.
If I have time I'll get the Reactive one thrown together tomorrow unless there's a whole horde of issues to resolve with this one.