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

Introducing not found error for config values #201

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

AuHau
Copy link
Member

@AuHau AuHau commented Jun 27, 2019

No description provided.

@AuHau AuHau force-pushed the feat/config_not_found_err branch from 314977c to cfcd0b0 Compare June 27, 2019 11:15
@AuHau AuHau requested a review from jacobheun August 6, 2019 15:10
License: MIT
Signed-off-by: Adam Uhlir <[email protected]>
@AuHau AuHau force-pushed the feat/config_not_found_err branch from cfcd0b0 to 9376013 Compare August 6, 2019 15:39
@AuHau
Copy link
Member Author

AuHau commented Aug 6, 2019

I have now realized that earlier I created issue #197 that propose the usage of interface-datastore.errors.notFound instead of this custom error.

@jacobheun what is your take on this?

@jacobheun
Copy link
Contributor

@AuHau they should probably be separate. While they both have the same error code at the moment, the missing config key is not a database error. There's no guarantee the database code won't change in the future, so keeping them separate feels like a safer way to avoid issues down the road.

The err-code module is already being used in other places in ipfs-repo, is there any particular reason you're using error classes instead? If not it would probably be good to use the former.

@AuHau
Copy link
Member Author

AuHau commented Aug 6, 2019

@jacobheun sounds good!

Well, it is a style of errors proposed in ipfs/js-ipfs#1746 that I silently highly prefer 😅 and I hope is the future in js-ipfs land. Should I refactor it nevertheless?

@jacobheun
Copy link
Contributor

My only real hesitation with including them is that we have 2 ways errors are being created now. I know there is another PR open that adds another code. Would you be up for submitting another PR to replace the rest of the err-code usage?

@AuHau
Copy link
Member Author

AuHau commented Aug 6, 2019

Sure! I will happily do that. It should not be even breaking change as the code properties will stay the same. I will make it happen tomorrow.

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.

2 participants