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

remove Get<T> extension method to make tests more clear #1063

Merged
merged 1 commit into from
Jan 26, 2016

Conversation

shiftkey
Copy link
Member

Fixes #1062

As part of #1062 I found that an extension methods for Get<T>(uri) is interfering with some of our unit tests - as it's an extension method contributors can't verify it was invoked correctly, and when that doesn't work blindly add the second parameter to get the test to pass.

This is done in a lot of tests, and it's a trivial extension, so why not just make it a feature of the interface?

I'm happy to leave the other methods in ApiExtensions as-is, they're not used as broadly.

cc @haacked for feels and any potential quirks about switching from extension method to interface method.

…ct method, removing the need for

additional nulls in tests
@haacked
Copy link
Contributor

haacked commented Jan 26, 2016

The only downside is that if you implement the interface, you only need to implement the one Get method and you know the extension method is implemented correctly with regard to that one method. Now, implementers have to implement both and implement them correctly.

Having said that, this really does clean up the tests and API and reduces pain and it's a really really simple method so I'm 👍

haacked added a commit that referenced this pull request Jan 26, 2016
remove Get<T> extension method to make tests more clear
@haacked haacked merged commit 615b690 into master Jan 26, 2016
@haacked haacked deleted the remove-trivial-extension-methods branch January 26, 2016 19:24
@shiftkey
Copy link
Member Author

@haacked yeah, agreed on the need to implement this explicitly. If we had multiple implementations I'd probably explore something else...

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.

2 participants