twindle-co / twindle

Twindle - an open source project for beginners. Converting twitter threads to pdf, epub, and mobi format to be read by Kindle.
https://www.twindle.co
MIT License
134 stars 133 forks source link

Meaningful failure messages #717

Closed PuruVJ closed 3 years ago

PuruVJ commented 3 years ago

twitter/api/helpers/fetch.js has this code

  // REVIEW WANTED: What should we do when its not 200? This below is just a workaround for now
  if (response.status !== 200) throw new ApiErrors.NetworkRequestError();

If the request fails, it throws the error that there's an error in the network, CHeck your network, etc. But it would give same error if the ID is invalid

image

How do we tackle this @Mira-Alf @johnjacobkenny?

sarathkumar6 commented 3 years ago

@PuruVJ What are the other error codes? Based on that we can come up with user friendly error messages.

500 - Sorry, our server is having issues. Please come back later 400 - Bad Request, possibly some useful information on what's wrong with the request. 401 - You're not authorized to access the app

@Mira-Alf @johnjacobkenny @proful

johnjacobkenny commented 3 years ago

Yeah, I think that this list looks good to start off with. In addition, we should handle any that the twitter API tells us to.

Proful commented 3 years ago

Looks good

However these are RESTful error codes. Relevant to CLI?

I don't remember any CLI throw or display these kind of errors.

As long as we use internally I am fine but we need to translate human readable error message to the CLI user.

PuruVJ commented 3 years ago

I'm thinking of errors like Invalid Bearer token, or unknown tweet, or something like that. The REST error codes can be answered with the NetworkError itself

johnjacobkenny commented 3 years ago

Mira is looking into it and will share her findings

palashmon commented 3 years ago

Maybe we can also start exploring other options also like: winston or morgan

Mira-Alf commented 3 years ago

200 response by Twitter

Non 200 response code by Twitter: Comprehensive list

The question now is should we classify all non 200 error codes returned by Twitter under a single error? Does it make sense to have too much detail? Like for example in the original message by @PuruVJ he had used "haha" as tweet id and that will throw a 400 error code. What information must be displayed to the user? Throwing a network issue does not make much sense in this case.

Inputs appreciated.

Mira-Alf commented 3 years ago

Maybe we can also start exploring other options also like: winston or morgan

Nice, so these winston and morgan are logger options? Good. If someone can pick this up and set it up, then I can implement it in all the error scenarios..

PuruVJ commented 3 years ago

@Mira-Alf Thanks. I've implemented most of these by testing the API with different paramters. Expect a PR soon.

Mira-Alf commented 3 years ago

Great @PuruVJ Thanks!

PuruVJ commented 3 years ago

Is this issue resolved fully? Can it be closed?