Throw Errors from Load Function
obermillerk opened this issue · 4 comments
Looking to use this library as I'll be loading in npy files. I was looking at the source code and noticed that you're catching potential errors in the load function and just printing them to console.error. I would like to suggest that those errors are allowed to propagate out of the function, so the developer can handle them instead of having them go to the console.
The callback might be an issue. My ideas on that would be to either add a second errorCallback, or a second error argument to the first callback. I'd be happy to put together a PR as well if that helps!
Edit: I should clarify, the second argument method I think should follow the error-first callback schema, so the result would then be in the second argument. This would be a change in the way the module works, and would possibly warrant a minor version bump if accepted. The errorCallback method would not change existing functionality as it could simply be omitted, which may be preferable.
This is an excellent point — a PR of your suggestion would be hugely appreciated!
Could you explain your preference for the error to be the first callback? Might make sense to keep backward-compatible ordering for now, but I can be convinced :D
My apologies, I guess my edit was unclear. I meant that in the case of adding a second argument to the callback, we should follow the error-first callback convention from before promises where there is a single callback that is passed two arguments. The first argument is any error that happened, or null if none happened in which case there's a second argument that is the normal result as is passed to the callback now. This would not be backward compatible.
If a second callback specifically for errors were added, I agree that would be better as a second callback as this would be backward compatible. I can go with this one since it sounds like your preference is not to break backwards compatibility. I can work on a PR tomorrow probably.
Aha! Now I understand. Got it, thank you for explaining!
My preference is (success, error) for now with the plan to change it in the next version roll.
it would be nice to preserve backward compatibility if that is acceptable for your use case. But hey, you're the one writing the PR ;)
I can certainly do that, I'll be using promises anyway.