KonstantinGasser / sherlock

easy to use and simple cli password manager
Apache License 2.0
3 stars 2 forks source link

check group exists before listing group's accounts #16

Closed amit-pub closed 3 years ago

amit-pub commented 3 years ago
KonstantinGasser commented 3 years ago

@amit-pub great, thanks for the PR! 😄 I will check it out starting next wee!

KonstantinGasser commented 3 years ago

@amit-pub I reviewed your changes and I am not quite sure why we would need a package errors. I understand the idea that it would be a central place to define errors. However, letting each package define its own errors comes with some advantages. For example if the package fs returns an error the caller can for sure say it won't be any error related to the let's say package internal. Where on the other hand with a global approach the fs could return a ErrWeakPassword.

If you had something else in mind with this you can open an issue and we can discuss thoughts in there 👍

amit-pub commented 3 years ago

Regarding package errors i was thinking, that we should have our own error-definitions used across the project, so that the errors that client in this case terminal-prompt will be seeing consistent errors & it would be helpful when we want to handle / parse errors from any call stacks within the code. Also, the custom error definitions were needed only at the user interfacing package, not in all package. So, fs pkg can still return the error that it wants, but the upper layer that is returning errors to users, should be standard one, hence this package errors is what i was thinking to have.

I think, this can be picked up later in the future when this matures gradually. So that standardization becomes priority.

With this, I'm closing this PR.