intercom / intercom-dotnet

Intercom API client library for .NET
https://developers.intercom.io/reference
Apache License 2.0
63 stars 53 forks source link

Company created_at incorrect type #48

Closed alcjamgit closed 6 years ago

alcjamgit commented 7 years ago

Hi,

Why is the data type for created_at int? This should be long as this is the Unix time stamp as per the api:

namespace Intercom.Data { public class Company : Model { public Company();

    public string company_id { get; set; }
    public int? created_at { get; set; }
    //Other properties
}

}

I checked the Java SDK and even the data type for that is long.

gbronzer commented 7 years ago

This is actually causing exceptions when updating companies. All the date properties should be longs.

khalilovcmd commented 7 years ago

Thank you @alcjamgit for reporting this! It has been discussed before here: https://github.com/intercom/intercom-dotnet/issues/46 I think the more accurate choice here would be UInt32

@gbronzer can you please elaborate of why is it causing exceptions?

gbronzer commented 7 years ago

I was attempted to use companyClient.Update, which returns the company as the result. The call works fine, but when it tried to map the result back to the company object it blew up on the Int cast. I believe it was because the particular company had a default created_at date in Intercom which was 1/1/1901 which converts to -2177452800. When that tried to cast to an Int, it exceeded the Int32 bounds.

I don't really agree that UInt32 makes sense because it means custom attributes with date format can never be earlier that epoch because those are negative.

khalilovcmd commented 7 years ago

@gbronzer I believe in this particular case, you are correct.

One side would argue:

Int32.Max in .NET is 2147483647. The current time since epoch is 1478623294. We will only reach the Int32.Max in year 2038.

As well as, Unix historically stores as Signed Int, and in some dbs TIMESTAMP datatype has 4 bytes.

I think we can just have it as long to serve all purposes!

Also, @gbronzer if you feel that you have any more changes, fixes or improvements, please send a PR right away 😃

gbronzer commented 7 years ago

@khalilovcmd I created a pull request for the long type changes.

mivra commented 7 years ago

Why the pull request to fix this bug has not been merged yet?? It's blocking us to use the SDK.

alcjamgit commented 7 years ago

@khalilovcmd - could you please check @gbronzer's pull request.

choran commented 7 years ago

Hi all, Apologies for not getting to this yet, we will review all dotnet PRs shortly and ensure they are reviewed more frequently in future Thanks Cathal

kmossco commented 6 years ago

This has been fixed recently: https://github.com/intercom/intercom-dotnet/blob/master/src/Intercom/Data/Company.cs#L14