Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wish we could tell if the append happened #21

Closed
jcorbin opened this issue Jun 28, 2017 · 1 comment · Fixed by #31
Closed

Wish we could tell if the append happened #21

jcorbin opened this issue Jun 28, 2017 · 1 comment · Fixed by #31
Assignees

Comments

@jcorbin
Copy link

jcorbin commented Jun 28, 2017

I've hit a surprising number of cases recently where I wished that multierr.Append would tell me if the append happened or not. It's not so much for dubious micro perf reasons (CPUs being cheap, fast, and plentiful these days), but more on account of simplifying control flow:

  • when I don't need to tell if it happened, I can use the inline form like err = multierr.Append(err, somethingFaliable())
  • but if I need to know if somethingFaliable failed or not in addition to collecting its error, then I have to break that apart, and do my own nil check
  • really the hardest part of this, as always, is choosing what to name someErr ;-)
package main

type T struct{}

type Scanable interface {
	Scan(...interface{}) bool
}

// harvestT loads T from some Scanable source. Since we really like T, we don't
// let mundane details like parsing errors get in our way; instead we continue
// on to harvest as much T as we can, collecting any errors that we encounter.
func harvestT(iter Scanable) ([]T, error) {
	var (
		mess   string // each string from the db
		datum  T      // each parsed item
		data   []T    // all items parsed
		retErr error  // any collected errors
	)

	// with current multierr
	for iter.Scan(&mess) {
		if parseErr := tryToParse(s, &datum); parseErr != nil {
			retErr = multierr.Append(retErr, parseErr)
			continue
		}
		data = append(data, datum)
	}

	// however what if we could:
	for iter.Scan(&mess) {
		retErr, failed = multierr.Appended(retErr, parseErr)
		if failed {
			continue
		}
		data = append(data, datum)
	}

	// or even "better" in this case:
	for iter.Scan(&mess) {
		retErr, failed = multierr.Appended(retErr, parseErr)
		if !failed {
			data = append(data, datum)
		}
	}

	// alternatively, if we used a *error:
	for iter.Scan(&mess) {
		if multierr.AppendInto(&retErr, parseErr) {
			continue
		}
		data = append(data, datum)
	}

	// which of course could be simplified in our singleton case (you wouldn't
	// want to do this if you had more than one faliable step in the loop
	// body):
	for iter.Scan(&mess) {
		if !multierr.AppendInto(&retErr, parseErr) {
			data = append(data, datum)
		}
	}

	return data, retErr
}
at15 added a commit to dyweb/gommon that referenced this issue Feb 25, 2018
at15 added a commit to dyweb/gommon that referenced this issue Feb 25, 2018
[errors] Replace pkg/errors Fix #54 

- multi error and wrap error
- multi error `Append(err error)` returns true if it got a non nil error uber-go/multierr#21
- `Wrap` would reuse stack if the underlying error has one pkg/errors#122, and if `Frames()` is not called, `runtime.Frames` would not be expanded to `[]runtime.Frame`
@abhinav
Copy link
Collaborator

abhinav commented Oct 30, 2019

I'm in favor of AppendInto.

func AppendInto(into *error, err error) (errored bool)

The surface area is small, and the possibility of misuse is acceptable.

A PR would be welcome.

abhinav added a commit that referenced this issue Nov 4, 2019
This adds an AppendInto function that behaves similarly to Append
except, it operates on a `*error` on the left side and it reports
whether the right side error was non-nil.

    func AppendInto(*error, error) (errored bool)

Making the left side a pointer aligns with the fast path of `Append`.

Returning whether the right error was non-nil aligns with the standard
`if err := ...; err != nil` pattern.

```diff
-if err := thing(); err != nil {
+if multierr.AppendInto(&err, thing()) {
   continue
 }
```

Resolves #21
abhinav added a commit that referenced this issue Nov 4, 2019
This adds an AppendInto function that behaves similarly to Append
except, it operates on a `*error` on the left side and it reports
whether the right side error was non-nil.

    func AppendInto(*error, error) (errored bool)

Making the left side a pointer aligns with the fast path of `Append`.

Returning whether the right error was non-nil aligns with the standard
`if err := ...; err != nil` pattern.

```diff
-if err := thing(); err != nil {
+if multierr.AppendInto(&err, thing()) {
   continue
 }
```

Resolves #21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants