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

Change type of date/time fields from 'string' to 'DateTimeOffset' #101

Merged
merged 1 commit into from
Oct 31, 2014

Conversation

ggrossman
Copy link
Contributor

Hi, this is my first PR to your repo, let me know if there are things you'd like done differently.

The Zendesk API v2 requires that most date/times be formatted as ISO8601. In the ZendeskApi_v2 library, the types of date/time fields is always string.

It would be helpful for it to be DateTime or DateTimeOffset so that developers don't pass incorrect date/time formats to the Zendesk API. For instance, there have been Zendesk API developers who have passed strings like Friday, September 9, 2014 8:03 AM for due_at to the Zendesk API.

It also is helpful for developers when receiving values back from the Zendesk API; if they're already of type DateTime or DateTimeOffset in the returned result, the developer doesn't have to worry about parsing the result.

I chose DateTimeOffset over DateTime since it includes timezone information and is the more modern choice in the .NET Framework. (Does this make sense?)

This is probably a breaking change so I suppose it'll have to be carefully considered. However, I think most developers would prefer to work with a real date/time object instead of a string in this case.

  • Changed date/time types from string to DateTimeOffset?
  • Added unit test for creating a ticket with DueAt setting
  • Add check for CreatedAt, UpdatedAt to ticket creation test

Add unit test for creating a ticket with DueAt setting
Add check for CreatedAt, UpdatedAt to ticket creation test
@eneifert eneifert mentioned this pull request Oct 11, 2014
j0emay referenced this pull request Oct 15, 2014
@eneifert
Copy link
Contributor

I have pulled this down and will release it at the end of October.

@eneifert eneifert merged commit cde6aa9 into Speedygeek:master Oct 31, 2014
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