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

Update company retrieve to match http api #472

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cullen-ambience
Copy link

Why?

Why are you making this change?

Company retrieve doesn't match the api here which can return either a Company or a Company list depending on whether the company_id or name param are specified.

How?

Technical details on your change

Changed the output of retrieve to return either Company or CompanyList. Added an or to check which parameters were passed and return the correct type.

@fern-support
Copy link
Collaborator

Thanks for the suggestion @cullen-ambience! Note that the SDK is generated from the OpenAPI spec sourced here.

In this case, there are two separate methods:

  1. client.companies.retrieve returns a Intercom.CompanyList.
  2. client.companies.find returns a single Company.

With this, I'd argue the current design is better. The user doesn't need to determine whether or not there is a single element or a list - they can just use the right method for the job.

cc @Eclairemoy for any additional context on the design of the API (just in case that doesn't cover it).

@cullen-ambience
Copy link
Author

Thanks for the suggestion @cullen-ambience! Note that the SDK is generated from the OpenAPI spec sourced here.

In this case, there are two separate methods:

  1. client.companies.retrieve returns a Intercom.CompanyList.
  2. client.companies.find returns a single Company.

With this, I'd argue the current design is better. The user doesn't need to determine whether or not there is a single element or a list - they can just use the right method for the job.

cc @Eclairemoy for any additional context on the design of the API (just in case that doesn't cover it).

Based on my understanding these Intercom APIs are doing two different things. Find uses intercom internal company id ("id" in the Company type) to retrieve a single company. Retrieve uses a number of params, one of which can be external company id ("company_id" in the Company type). It sounds like the issue may be with the underlying OpenAPI design.

Regardless, I'm seeing Company formatted data being returned from company.retrieve when company_id is specified in the params, which causes a type conflict in my app that expects CompanyList. I can recast the type and get around it for now, but thought it would be better to flag it for you all :).

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