github / gh-migration-analyzer

MIT License
64 stars 21 forks source link

Fix ReferenceError from invalid function parameters #9

Closed Aashishkebab closed 2 years ago

Aashishkebab commented 2 years ago

The handleStatusError() function doesn't take in the parameter for the exception itself, which is called "err".

However, the function attempts to use "err" later on despite it not existing, causing a ReferenceException. This prevents the application from displaying the real error and instead shows that exception.

Additionally, the spinner.fail() call does not display enough information to the user. I added a console.log() so that the full exception is output.

This fixes #8.

ystndr commented 2 years ago

@slenguyen Can we have this reviewed or get directed to someone who would? Thanks!

slenguyen commented 2 years ago

@ystndr Apologies, and thanks for the ping. Turns out I wasn't watching this repo, so I didn't get a notification on the PR. Fixed that!

I see we've added a dependency (minimist), and I wanted to check to see how that's being used -- or if that was just for debugging? Could I also trouble you to remove the console.log? Thanks!

Also: Thanks so much for the contribution!

Aashishkebab commented 2 years ago

@ystndr Apologies, and thanks for the ping. Turns out I wasn't watching this repo, so I didn't get a notification on the PR. Fixed that!

I see we've added a dependency (minimist), and I wanted to check to see how that's being used -- or if that was just for debugging? Could I also trouble you to remove the console.log? Thanks!

Also: Thanks so much for the contribution!

Mininist was auto added when I ran npm install. I did not add it manually.

The console.log shows useful information for the error. Otherwise, you don't get a useful error output. The "spinner" outputs this unusable image: IMG_0076.PNG

Whereas the console outputs this much more helpful one: IMG_0077.jpg

The Console output is the only reason we were able to figure out that the issue was a self-signed certificate, as "Server Side Error.itHub" doesn't really tell me what's wrong.

We were able to resolve it by disabling TLS temporarily.

slenguyen commented 2 years ago

Thanks for the details!