#248: Standardize error messages across codebase
@soumya-codes would like to discuss once regarding the changes for Standardization of errors issue Wanted to check on few points:
- Should we follow a
enumlike structure to standardise use of Command names, info etc. across the codebase? - Redis follows an approach where it passes the
clientpointer across which holds all details of the command being executed? Should we also try to follow the same? As currently under each method we need to hardcode the command name when throwing error.
Please review once haven't changed all error messages as wanted to check if these changes look good or we need to create some other framework.
Should we follow a enum like structure to standardise use of Command names, info etc. across the codebase?
Enum is a great option when we use error codes. Since we are not dealing with error codes here it may not be necessary.
Redis follows an approach where it passes the client pointer across which holds all details of the command being executed? Should we also try to follow the same? As currently under each method we need to hardcode the command name when throwing error.
-
Passing client pointer(containing additional non-necessary stuff) is neither safe nor an idiomatic approach in GO. We should only pass the minimum possible information as required. It is ok and IMO good to hard code the method-names. Creating string constants for each command type seems to be an over-optimization currently as we use it currently in two places.
-
I really like the way you have standardized the
DiceErrorand made it easy for the developers to deal with both static and dynamic errors.
@soumya-codes @JyotinderSingh please review once
@lucifercr07 Please check the linter warnings. You can easily fix them. Thanks
Looks like the changes went out of sync, could you please rebase and check for any new errors that may have been added? I'll try to merge the PR soon after that so we don't have to keep rebasing.
There seems to be a test failure, could you please check? Seems like a trivial fix.
@JyotinderSingh Have pushed the fix on my fork branch, was this PR accidentally closed? Shall I raise a new PR?
@JyotinderSingh Have pushed the fix on my fork branch, was this PR accidentally closed? Shall I raise a new PR?
Might have closed it by mistake, sorry!
@JyotinderSingh build passed, please merge.
Thanks a ton for this massive effort!
#248