dice icon indicating copy to clipboard operation
dice copied to clipboard

#248: Standardize error messages across codebase

Open lucifercr07 opened this issue 1 year ago • 4 comments

lucifercr07 avatar Aug 09 '24 11:08 lucifercr07

@soumya-codes would like to discuss once regarding the changes for Standardization of errors issue Wanted to check on few points:

  1. Should we follow a enum like structure to standardise use of Command names, info etc. across the codebase?
  2. 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.

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.

lucifercr07 avatar Aug 09 '24 11:08 lucifercr07

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.

  1. 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.

  2. I really like the way you have standardized the DiceError and made it easy for the developers to deal with both static and dynamic errors.

soumya-codes avatar Aug 09 '24 12:08 soumya-codes

@soumya-codes @JyotinderSingh please review once

lucifercr07 avatar Aug 16 '24 16:08 lucifercr07

@lucifercr07 Please check the linter warnings. You can easily fix them. Thanks

AshwinKul28 avatar Aug 18 '24 12:08 AshwinKul28

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.

JyotinderSingh avatar Aug 21 '24 11:08 JyotinderSingh

There seems to be a test failure, could you please check? Seems like a trivial fix.

JyotinderSingh avatar Aug 21 '24 11:08 JyotinderSingh

@JyotinderSingh Have pushed the fix on my fork branch, was this PR accidentally closed? Shall I raise a new PR?

lucifercr07 avatar Aug 21 '24 11:08 lucifercr07

@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 avatar Aug 21 '24 11:08 JyotinderSingh

@JyotinderSingh build passed, please merge.

lucifercr07 avatar Aug 21 '24 12:08 lucifercr07

Thanks a ton for this massive effort!

JyotinderSingh avatar Aug 21 '24 12:08 JyotinderSingh

#248

JyotinderSingh avatar Aug 21 '24 12:08 JyotinderSingh