show all diags on hard error, remove uses of pkg/errors - #73
Conversation
Signed-off-by: gotwarlost <krishnan.anantheswaran@elastic.co>
| case *time.ParseError: | ||
| // If err is a time.ParseError then its string representation is not | ||
| // appropriate since it relies on details of Go's strange date format | ||
| // representation, which a caller of our functions is not expected | ||
| // to be familiar with. | ||
| // | ||
| // Therefore we do some light transformation to get a more suitable | ||
| // error that should make more sense to our callers. These are | ||
| // still not awesome error messages, but at least they refer to | ||
| // the timestamp portions by name rather than by Go's example | ||
| // values. | ||
| if err.LayoutElem == "" && err.ValueElem == "" && err.Message != "" { | ||
| // For some reason err.Message is populated with a ": " prefix | ||
| // by the time package. | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp%s", err.Message) | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: %w", err) | ||
| } | ||
| var what string | ||
| switch err.LayoutElem { | ||
| case "2006": | ||
| what = "year" | ||
| case "01": | ||
| what = "month" | ||
| case "02": | ||
| what = "day of month" | ||
| case "15": | ||
| what = "hour" | ||
| case "04": | ||
| what = "minute" | ||
| case "05": | ||
| what = "second" | ||
| case "Z07:00": | ||
| what = "UTC offset" | ||
| case "T": | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: missing required time introducer 'T'") | ||
| case ":", "-": | ||
| if err.ValueElem == "" { | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: end of string where %q is expected", err.LayoutElem) | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: end of string where %q is expected: %w", err.LayoutElem, err) | ||
| } else { | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: found %q where %q is expected", err.ValueElem, err.LayoutElem) | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: found %q where %q is expected: %w", err.ValueElem, err.LayoutElem, err) | ||
| } | ||
| default: | ||
| // Should never get here, because time.RFC3339 includes only the | ||
| // above portions, but since that might change in future we'll | ||
| // be robust here. | ||
| what = "timestamp segment" | ||
| } | ||
| if err.ValueElem == "" { | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: end of string before %s", what) | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: end of string before %s: %w", what, err) | ||
| } else { | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: cannot use %q as %s", err.ValueElem, what) | ||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: cannot use %q as %s: %w", err.ValueElem, what, err) | ||
| } | ||
| } | ||
| return time.Time{}, err |
There was a problem hiding this comment.
I wonder if it make sense to also do the change in parseTimestamp.
In this function, the case *time.ParseError exists, per its own comment, to avoid leaking time.RFC3339's internal reference layout (2006-01-02T15:04:05Z07:00) into user-facing error messages: "Therefore we do some light transformation to get a more suitable error... these are still not awesome error messages, but at least they refer to the timestamp portions by name rather than by Go's example values."
datetime_test.go now expects:
not a valid RFC3339 timestamp: cannot use "bloop" as year: parsing time "bloop" as "2006-01-02T15:04:05Z07:00": cannot parse "bloop" as "2006"
instead of the old, clean:
not a valid RFC3339 timestamp: cannot use "bloop" as year
Also, there is now a stale comment line 156-158 ("For some reason err.Message is populated with a ': ' prefix") is also now describing a field (err.Message) that this branch no longer uses at all.
Should the comment be updated or the change reverted?
Proposed change
Keep using the derived, human-readable string (err.Message / the constructed what-based message) as the sole content of these errors ; don't also wrap the raw *time.ParseError with %w in this function, since the whole point of the branch is to replace that raw error's text:
if err.LayoutElem == "" && err.ValueElem == "" && err.Message != "" {
return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp%s", err.Message)
}
...
case ":", "-":
if err.ValueElem == "" {
return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: end of string where %q is expected", err.LayoutElem)
}
return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: found %q where %q is expected", err.ValueElem, err.LayoutElem)
...
if err.ValueElem == "" {
return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: end of string before %s", what)
}
return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: cannot use %q as %s", err.ValueElem, what)
| } | ||
| finalErr = errors.Join(errs...) |
There was a problem hiding this comment.
L30 is now a dead assignment: sortDiagsBySeverity(diags) result is discarded before use. It's harmless today (sort mutates in place) but confusing and fragile. A future refactor that changes sortDiagsBySeverity to return a copy (a very natural change for a function named sortX, which normally implies non-mutating) would silently break ordering with no signal at the call site.
Proposed change
if finalErr != nil {
diags, ok := finalErr.(hcl.Diagnostics)
if ok {
sortDiagsBySeverity(diags)
var errs []error
for _, diag := range diags {
errs = append(errs, diag)
}
finalErr = errors.Join(errs...)
}
}2 participants
Same PR as https://github.com/crossplane-contrib/function-hcl/pulls but in monorepo directory structure.