intercom / intercom-dotnet

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

Missing Count property on Segment #122

Closed ThomasArdal closed 6 years ago

ThomasArdal commented 6 years ago

In the API, Segments have a count attribute. This attribute is never mapped to a property on the Segment object. I've talked with Intercom support, who tell me that it is because count wasn't available in the previous versions of the API.

kmossco commented 6 years ago

You're right, this is missing. We are having a few woes with the transition to DotnetStandard but I'll try to squeeze this in asap.

image

However if anyone has the time to send us a PR I'll be happy to review it.

ThomasArdal commented 6 years ago

How would you prefer the include_count parameter to be set?

Could be always:

result = Get<Segment>(parameters: new Dictionary<string, string> { { "include_count", "true" } }, resource: SEGMENTS_RESOURCE + Path.DirectorySeparatorChar + id);

Through a parameter on the View-method:

public Segment View(String id, includeCount = false)

Or something third?

ThomasArdal commented 6 years ago

@kmossco

kmossco commented 6 years ago

@ThomasArdal so sorry for the delay here, completely missed this one. 😞 I think that the second option would make more sense as it's the pattern we already have for other endpoints like Scroll:

https://github.com/intercom/intercom-dotnet/blob/04fc9d7082641bc6089070d9147e5f6b1feef1eb/src/Intercom/Clients/ContactsClient.cs#L134-L145

How does that sound?

ThomasArdal commented 6 years ago

No problem :smile:

There you go: https://github.com/intercom/intercom-dotnet/pull/126

kmossco commented 6 years ago

This is awesome! 🎉 Thank you so much. We are having a few issues with our Nuget package build and so PRs are a bit delayed, but as soon as we fix that I'll review your PR and add it to the next release. 👍

kmossco commented 6 years ago

Closed with release 2.1.0.