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

fetchById throw error if document does not exists by given id #340

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fetchById throw error if document does not exists by given id #340

wants to merge 1 commit into from

Conversation

moreorover
Copy link

I had a quick look at the source, I think this should solve the issue, however, testing is required.

@louisameline
Copy link
Collaborator

I haven't tested, but from what I read, the function previously returned a string in case of error if logging is enabled, and now would throw an exception. Am I right?
It's cleaner of course but this would be a breaking change for the people who leave logging enabled in production. Maybe you could change your PR to throw only if logging is disabled?
I know it sounds stupid and returning strings is not the way to go, but the lib is going to be rewritten soon with proper error handling everywhere, so we might not want to break backward compatibility by trying to fix things now.

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.

None yet

2 participants