go-maildir: collect all parse errors during Walk v1 PROPOSED

Bence Ferdinandy: 1
 collect all parse errors during Walk

 1 files changed, 8 insertions(+), 5 deletions(-)

Thanks for the ping, and sorry for the delay.

Bence Ferdinandy <bence@ferdinandy.com>

9 months ago

No worries, I wasn't exactly super speedy in getting back here myself ^^

Next

When you sent this patch, I've reworked it a bit [1] so that we don't
create long chains of nested errors (instead a single errors.Join),
fix a test failure and drop the error prefix (already included in the
existing errors).

Bence Ferdinandy <bence@ferdinandy.com>

9 months ago

That indeed looks better!

Next

The reason I haven't pushed this is that I'm still wondering how to
handle errors from the user-provided callback. Right now callers expect
that an error returned from the callback will be passed as-is, but with
this patch it's no longer the case. I don't want to special-case the
situation where there is no format error but there is a callback error,
because this would result in unreliable error handling in the callers
(it would appear to work if there is no format failure, but would break
as soon as there is one).

Probably the way to go is to always wrap the error with errors.Join as
done in the branch, and document this as a breaking change?

Bence Ferdinandy <bence@ferdinandy.com>

9 months ago

I think yes, you are right, this is probably the best solution here. Users can
still parse the returned error themselves, so it shouldn't be a big deal to
update any downstream code.

Thanks,
Bence

[1]: https://github.com/emersion/go-maildir/compare/master...walk-err

I've added some docs and pushed the patch.

Bence Ferdinandy <bence@ferdinandy.com>

8 months ago

Great, thanks! Could I also convince you to tag a release for this? Unless of
course you see one coming soonish anyway. I could then patch aerc to use this.

Thanks,
Bence

Yes, done:
https://github.com/emersion/go-maildir/releases/tag/v0.5.0

[PATCH go-maildir] collect all parse errors during Walk Export this patch

Collect all the parse errors during Walk and only expicitly fail, when
the callback errs. Return the Joined errors along with the file names.

Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
---

Bence Ferdinandy <bence@ferdinandy.com>

11 months ago

just pinging this, to see if you have any thought. As a reminder, it came up in
this issue: https://github.com/emersion/go-maildir/issues/23

Thanks,
Bence

 maildir.go | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/maildir.go b/maildir.go
index dc3d9d2..d537405 100644
--- a/maildir.go
+++ b/maildir.go
@@ -82,7 +82,6 @@ func (d Dir) Unseen() ([]string, error) {
 		} else if err != nil {
 			return keys, err
 		}
-
 		for _, n := range names {
 			if n[0] == '.' {
 				continue
@@ -168,6 +167,7 @@ func (d Dir) Walk(fn func(key string, flags []Flag) error) error {
 	}
 	defer f.Close()
 
+	var errs error
 	for {
 		names, err := f.Readdirnames(readdirChunk)
 		if errors.Is(err, io.EOF) {
@@ -183,21 +183,24 @@ func (d Dir) Walk(fn func(key string, flags []Flag) error) error {
 
 			key, err := parseKey(n)
 			if err != nil {
-				return err
+				errs = errors.Join(errs, fmt.Errorf("%s: %w", n, err))
+				continue
 			}
 
 			flags, err := parseFlags(n)
 			if err != nil {
-				return err
+				errs = errors.Join(errs, fmt.Errorf("%s: %w", n, err))
+				continue
 			}
 
 			if err := fn(key, flags); err != nil {
-				return err
+				errs = errors.Join(errs, fmt.Errorf("%s: %w", n, err))
+				return errs
 			}
 		}
 	}
 
-	return nil
+	return errs
 }
 
 // Keys returns a slice of valid keys to access messages by.
-- 
2.43.0

Bence Ferdinandy <bence@ferdinandy.com>

11 months ago

Hey,