FeedHive / twitter-api-client

A user-friendly Node.js / JavaScript client library for interacting with the Twitter API.
MIT License
948 stars 84 forks source link

Wrong `UsersShowParams` interface #11

Closed lcswillems closed 4 years ago

lcswillems commented 4 years ago

The current UsersShowParams is wrong:

export interface UsersShowParams {
    /**
    * The ID of the user for whom to return results. Either an id or
  screen_name is required for this method.

    */
    user_id: string | number;
    /**
    * The screen name of the user for whom to return results. Either
  a id or screen_name is required for this method.

    */
    screen_name: string;
    /**
    * The entities node will not be included when set to false.
    */
    include_entities?: boolean;
}

It says user_id or screen_name is required but both are currently required.

SimonHoiberg commented 4 years ago

Hi @lcswillems Nice catch - thanks for creating an issue!

Will take care of this asap :+1:

SimonHoiberg commented 4 years ago

This task is up for grabs during Hacktoberfest :raised_hands:

If you want to work on this task, please claim it here in the comments. Feel free to ask any questions here as well :blush:

lcswillems commented 4 years ago

@Silind, sorry having been direct in my issue, I was coding and discovered this bug so I quickly filled it. I would like to thank you a lot for your library! It made me save a lot of time with your typings :)

iliran11 commented 4 years ago

Hey @lcswillems @Silind . Not sure I understood the bug. On twitter docs, it says the same thing as in the interface: https://developer.twitter.com/en/docs/twitter-api/v1/accounts-and-users/follow-search-get-users/api-reference/get-users-show

$ curl --request GET 
    --url 'https://api.twitter.com/1.1/users/show.json?screen_name=twitterdev' 
    --header 'authorization: Bearer <bearer>'

Can you elaborate more?

SimonHoiberg commented 4 years ago

Hi @iliran11 Yeah, so it's an unfortunate occasion because both parameters are flagged as required but in the description, it actually says "Either an id or screen_name is required for this method".

So it means that you're supposed to provide either and id or a screen_name parameter. But in the twitter-api-spec.yml, these are both set to required, which is what the UsersShowParams interface is based on.

There's a way to make a type in TypeScript, that satisfies the constraint that either one of them is provided, but it becomes hard to infer when this type should be used according to the Twitter Docs. And the generators don't yet support this concept, so it'll need to be implemented.

I think it's a great feature for the future, but for now, I think the best solution is to simply make them both optional. This does allow the consumer to call this endpoint with none of them provided, but if so, an error will be thrown from the Twitter API.

iliran11 commented 4 years ago

Got it. I can take it too. @Silind, can you make me a collaborator so I can open a branch on this repository? Gonna be more convenient than working on a fork. I'll open PR against the development branch.

SimonHoiberg commented 4 years ago

@iliran11 Yes, totally! I sent you an invite.

SimonHoiberg commented 4 years ago

Fixed in v. 1.0.2.