refactor: Add Design.md to explain refactor by anthony-treuillier-scality · Pull Request #15 · scality/go-errors

anthony-treuillier-scality · 2025-12-10T08:48:52Z

No description provided.

ezekiel-alexrod · 2025-12-10T14:23:56Z

DESIGN.md

+* WithDetail: provide a message that details the error
+* WithProperty: provide a key/value pair for additional informations (filename, path, username, ... )
+* WithIdentifier: used to provide an identifer, it could be concatenated with other idenfier from previous calls from subfunctions (See example below for clarity)


I would put the With<Name> in verbatim style

ezekiel-alexrod · 2025-12-10T14:24:29Z

DESIGN.md

+Signature could be as follow
+
+```go
+errors.WithIdentifier(int): 


You let an ending semicolon there

ezekiel-alexrod · 2025-12-10T14:24:44Z

DESIGN.md

+```go
+errors.WithIdentifier(int): 
+errors.WithDetail(string)
+errors.WithDetailf(string,  ...any)


an extra space is here

ezekiel-alexrod · 2025-12-10T14:26:01Z

DESIGN.md

+* WithDetail: provide a message that details the error
+* WithProperty: provide a key/value pair for additional informations (filename, path, username, ... )
+* WithIdentifier: used to provide an identifer, it could be concatenated with other idenfier from previous calls from subfunctions (See example below for clarity)
+
+Signature could be as follow
+
+```go
+errors.WithIdentifier(int): 
+errors.WithDetail(string)
+errors.WithDetailf(string,  ...any)
+errors.WithProperty(string, any)
+```


I would put them in the same order

ezekiel-alexrod · 2025-12-10T14:27:22Z

DESIGN.md

+```
+forbidden (19-12-2): permission denied: missing required role: File='test.txt', User='john.doe', Role:'Reader', at=(func='main.appel3', file='main.go', line='270')
+```
+
+```go
+package main
+
+import (
+    "fmt"
+
+    errors "github.com/scality/go-errors"
+)
+
+var ErrForbidden = errors.New("forbidden")
+
+func main(){
+    err := appel1()
+    fmt.Println(err)
+}
+func appel1() error {
+	return errors.Wrap(
+        appel2(),
+        errors.WithIdentifier(19),
+    )
+}
+
+func appel2() error {
+	return errors.Wrap(
+        appel3(),
+        errors.WithDetail("missing required role"),
+        errors.WithProperty("Role", "Reader"),
+        errors.WithProperty("User", "john.doe"),
+        errors.WithIdentifier(12),
+    )
+}
+
+func appel3() error {
+    // Something went wrong here
+	return errors.Wrap(
+        ErrForbidden,
+        errors.WithIdentifer(2),
+        errors.WithDetail("permission denied"),
+		errors.WithProperty("File", "test.txt"),
+    )
+}
+```


To me, I think it's better to put the error result after the code.

also missing a new line between main and appel1
Also also, I would rename appelX by funcX or foo bar baz, etc.

vdaviot · 2026-03-04T08:58:10Z

DESIGN.md

+    err := appel1()
+    fmt.Println(err)
+}
+func appel1() error {
+	return errors.Wrap(
+        appel2(),


call instead of appel 👍

eg-ayoub · 2026-03-04T09:18:19Z

DESIGN.md

+# Variadic Options
+The variadic options would be the following one
+
+* `WithDetail`/`WithDetailf`: provide a message that details the error
+* `WithProperty`: provide a key/value pair for additional informations (filename, path, username, ... )
+* `WithIdentifier`: used to provide an identifer, it could be concatenated with other idenfier from previous calls from subfunctions (See example below for clarity)


at the risk of being redundant I would like to ask about the naming of these options,
using the keyword "with" I would expect something like:

something.WithSomethingElse(.....)

is there a reason we don't call these options just Detail,Property, ...

func appel1() error { return errors.Wrap( appel2(), errors.Identifier(19), ) }

This is just a common pattern among some broadly used library, that I personnaly like a lot

eg-ayoub · 2026-03-04T09:21:30Z

DESIGN.md

small comment otherwise I completely agree with this design.
keeping the Identifiers vague as a responsibility to the user is a good touch.
do we plan on having helper functions for this library to ease error parsing ?
for example HasIdentifier, HasProperty, GetProperty
are properties guaranteed to be unique ?
thanks @anthony-treuillier-scality 👍🏼

TeddyAndrieux · 2026-03-04T09:32:22Z

DESIGN.md

@@ -0,0 +1,92 @@
+Design for go-errors refactor


Nit: To me it's weird to have this line here

We can consider it as a title, but I will remove it

TeddyAndrieux · 2026-03-04T09:33:07Z

DESIGN.md

+Design for go-errors refactor
+
+# Introduction
+The main objective of this refactor is to ease the usage of go-errors library, with key points


I would prefer we describe the "final view" of the lib and not talk about refactor here (so that it can be read at any time once it's merged)

but i am just talking about refactor

Signed-off-by: Anthony TREUILLIER <anthony.treuillier@scality.com>

anthony-treuillier-scality requested a review from a team as a code owner December 10, 2025 08:48

ezekiel-alexrod reviewed Dec 10, 2025

View reviewed changes

anthony-treuillier-scality force-pushed the improvement/refactor branch from 68a4669 to 25d6c21 Compare December 12, 2025 06:18

vdaviot approved these changes Mar 4, 2026

View reviewed changes

eg-ayoub reviewed Mar 4, 2026

View reviewed changes

TeddyAndrieux reviewed Mar 4, 2026

View reviewed changes

anthony-treuillier-scality force-pushed the improvement/refactor branch 2 times, most recently from 69670bf to fe83fa3 Compare March 5, 2026 05:51

refactor: Add Design.md to explain refactor

0c5125a

Signed-off-by: Anthony TREUILLIER <anthony.treuillier@scality.com>

anthony-treuillier-scality force-pushed the improvement/refactor branch from fe83fa3 to 0c5125a Compare March 5, 2026 07:45

TeddyAndrieux approved these changes Mar 5, 2026

View reviewed changes

Conversation

anthony-treuillier-scality commented Dec 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants