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

Better error handling #21

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Technohacker
Copy link

@Technohacker Technohacker commented Jun 12, 2019

For issue #5
Functions now throw Error objects instead of returning nulls

Potential issues:

  • Errors from request-promise and airbnbapi are both given back to the caller. More code may be necessary to give a unified interface
  • This would very likely be a breaking API change

This PR isn't intended to be merged right away until reviewed for more issues

Allows testing for Promise rejections cleanly
This makes failure cases more clear to the calling code
Check for thrown Error()s instead of nulls.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 50

  • 99 of 119 (83.19%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+6.9%) to 77.358%

Changes Missing Coverage Covered Lines Changed/Added Lines %
build/main.js 98 118 83.05%
Files with Coverage Reduction New Missed Lines %
build/log.js 1 47.83%
Totals Coverage Status
Change from base Build 49: 6.9%
Covered Lines: 217
Relevant Lines: 278

💛 - Coveralls

@zxol
Copy link
Owner

zxol commented Jun 12, 2019

That's brilliant! Thank you!

It looks like you may have edited the build files instead of the pre-babelised source code in the src directory. Totally understandable as I have neglected to write a contribution guide. My apologies.

How would you like to proceed?

@Technohacker
Copy link
Author

I've updated both (source then babelize), it's just that I didn't notice a gitignore for the build directory so I thought you kept both updated together 😅

The only commit that affects the build files would be the last one so the rest can be used instead :D

@zxol
Copy link
Owner

zxol commented Jun 12, 2019

Ahh sorry, Understood.

@zxol
Copy link
Owner

zxol commented Jun 12, 2019

Did you write a script to help you refactor the code? I'm curious about your approach to the problem.

@Technohacker
Copy link
Author

To be honest I was changing lines manually 🤣
I used Atom so for a majority of the lines I used multi-cursor editing :)

@zxol
Copy link
Owner

zxol commented Jun 12, 2019

Nice :)

@zxol
Copy link
Owner

zxol commented Jun 12, 2019

I'll review this carefully and try to make some additions as well, prior to merging. Perhaps we should notify the users about an upcoming API surface change, too.

@Technohacker
Copy link
Author

Yes that's a good idea. I'll keep building with the API and add more endpoints if necessary so all of it can be made into one release :)

@zxol
Copy link
Owner

zxol commented Jun 12, 2019

Sounds good. I'm really glad to have a collaborator on the project.

@Jacob-Cozens
Copy link

Very cool.

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.

4 participants