Implement interface #57
Conversation
|
Can one of the admins verify this patch? |
|
Hey @danny-may thanks for the contribution! I’ll get someone who knows more .NET than me to take a look at it. |
|
@govuk-notify-jenkins test this please |
There was a problem hiding this comment.
This PR looks absolutely sensible. Would it be possible to merge the commits into one and update the description with what you've written in the PR description? @danny-may
Thanks for taking the time to make this improvement 👍
src/Notify/Notify.csproj
Outdated
| @@ -3,7 +3,7 @@ | |||
| <PropertyGroup> | |||
| <RestoreProjectStyle>PackageReference</RestoreProjectStyle> | |||
| <TargetFrameworks>netstandard2.0;netcoreapp2.0;net462</TargetFrameworks> | |||
| <Version>2.2.0</Version> | |||
| <Version>2.2.1</Version> | |||
There was a problem hiding this comment.
This should be a minor update rather than a patch:
2.3.0
|
Im not sure how to merge the commits without re-doing the PR/branch. Is a squash merge not possible from your end? EDIT: Never mind, I figured it out! |
The NotificationClient class does not implement INotificationClient. Being unable to use an interface in the place of the NotificationClient makes unit testing rather painful, as we have to instead mock the IHttpClient. Updated version to 2.3.0
|
@govuk-notify-jenkins test this please |
|
Hey @danny-may, @karlbaker02 thanks again for your work on this. Going to merge now… |
|
No problem, happy to help 😄 |
What problem does the pull request solve?
The NotificationClient did not implement the INotificationClient interface. Being unable to use an interface in the place of the NotificationClient makes unit testing rather painful, as we have to instead mock the IHttpClient.
Checklist
I dont feel this is applicable as no functional code has changed
README.md, CHANGELOG.md and CONTRIBUTING.md)I am unsure if any doccumentation would need to change as the usage of the NotificationClient is unchanged
src/Notify/Notify.csproj)