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

Unit tests for IDatumConverter implementations #140

Merged
merged 11 commits into from
Sep 29, 2013

Conversation

tetious
Copy link
Collaborator

@tetious tetious commented Sep 28, 2013

This should fix #60.

Please let me know if you think anything should be added/changed.

Also, I added some Datum generation extension methods to reduce repetition. (DatumHelper)

@mfenniak
Copy link
Owner

Hey @tetious, this all looks good... except the DatumHelper extensions. I'm really not a fan of these extension methods for three reasons; (a) the pollute the global namespace by adding extension methods on primitives, they'd constantly be showing up in intellisense for library consumers, (b) they're useless to consumers of the library because they expose Datums, which aren't a type the consumer would use, and (c) they don't have all the same logic checks that the primitive datum converters have, so they're inconsistent and in the case of the long type potentially dangerous.

@tetious
Copy link
Collaborator Author

tetious commented Sep 29, 2013

Yeah, that makes sense. Would you object if I instead made them internal to the .Test assembly? Or would that still be annoying?

If so, I'll just make them static helpers in the test assembly.

@mfenniak
Copy link
Owner

Internal to the Test assembly sounds good to me. :-)

@tetious
Copy link
Collaborator Author

tetious commented Sep 29, 2013

Done. :)

@mfenniak
Copy link
Owner

Seems like teamcity.codebetter.com is offline right now, so I ran the updated test suite myself. Looks good. Merging. Thanks Greg. :-)

mfenniak added a commit that referenced this pull request Sep 29, 2013
Unit tests for IDatumConverter implementations, fixes #60
@mfenniak mfenniak merged commit 046acca into mfenniak:master Sep 29, 2013
mfenniak added a commit that referenced this pull request Sep 29, 2013
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.

Write unit tests for IDatumConverter implementations
2 participants