Skip to content

Better error handling#21

Open
Technohacker wants to merge 5 commits into
zxol:masterfrom
Technohacker:master
Open

Better error handling#21
Technohacker wants to merge 5 commits into
zxol:masterfrom
Technohacker:master

Conversation

@Technohacker

@Technohacker Technohacker commented Jun 12, 2019

Copy link
Copy Markdown

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
Copy Markdown

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

zxol commented Jun 12, 2019

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

zxol commented Jun 12, 2019

Copy link
Copy Markdown
Owner

Ahh sorry, Understood.

@zxol

zxol commented Jun 12, 2019

Copy link
Copy Markdown
Owner

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

@Technohacker

Copy link
Copy Markdown
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

zxol commented Jun 12, 2019

Copy link
Copy Markdown
Owner

Nice :)

@zxol

zxol commented Jun 12, 2019

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

zxol commented Jun 12, 2019

Copy link
Copy Markdown
Owner

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

@Jacob-Cozens

Copy link
Copy Markdown

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