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

Make fault less opinionated on displaying "stack" trace and error messages together #11

Closed
mec07 opened this issue Nov 18, 2022 · 7 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@mec07
Copy link

mec07 commented Nov 18, 2022

It is great that the final err.Error() returns the "stack" lines and the error messages intermingled. It is quite opinionated though. Users might want them separately, so adding some helper functions to Chain could be good, like chain.Stack() to get the "stack" trace (separate from the errors) and chain.Error() to get just the error messages without the "stack" trace.

It might actually make more sense to just make err.Error() just return the error messages as per normal, like it would with normal golang error wrapping. Then chain.String() returns the combined format of "stack" lines plus error messages and then add a helper function like chain.Stack() to return just the "stack" lines.

@Southclaws
Copy link
Owner

err.Error() should just give you the old simple xyz: abc: foo: bar: root error message strings glued together with : . But what's going on here I think is my Format implementation is bad. It's not respecting the rune argument.

Some libraries (like uber-go/fx) are opionionated about how they print errors, fx always uses %+v which triggers the Format method to be called with the rune v with the + modifier.

Others may just use %v which, really should just do what Error() does.

Definitely a lot to be cleaned up here before a v1! Thanks for raising this.

Alternatively, I could just ditch Format completely and let the user do what they want to do with the chain data structure.

@mec07
Copy link
Author

mec07 commented Nov 18, 2022

Ah damn! I realise that I had a major misunderstanding of the package because I was just messing about with it in a play.golang.org session and was using fmt.Println(err). So I saw err.Format(), and assumed it was err.Error(). My bad 😓 ! I've updated the play.golang.org session to actually use logging and the package is perfect as it is (i.e. I don't think it is too opinionated anymore): https://go.dev/play/p/5cpmQ594qxT

I think the reason why I thought it would look like a mix of both types is because the README file doesn't actually differentiate between fmt.Println(err) and err.Error(). To be honest the example following the words "A fault stack trace looks like this:" made me think that err.Error() caused that and I confirmed it (incorrectly, lol) in play.golang.org using fmt.Println(err).

My only recommendations would be to:

  • add a helper function to Chain to extract all the locations, e.g. func (c *Chain) Stack() []string
  • update the README "A fault stack trace looks like this:" --> "A fault stack trace looks like this (i.e. fmt.Println(err)):"
  • update the README to include an example of err.Error() as well as fmt.Println(err).
  • update the README to mention the new chain.Stack() helper function.

@Southclaws
Copy link
Owner

Ah good thanks for clarifying that! This is definitely a problem in communicating the behaviour in the documentation.

I'll definitely clarify the behaviour around this and also get a better .Format() implementation sorted too that respects the various modifier runes of %v.

@Southclaws
Copy link
Owner

Re: Format/%+v behaviour: #17

@mec07
Copy link
Author

mec07 commented Nov 21, 2022

Great! Thanks!

@Southclaws Southclaws reopened this Nov 30, 2022
@Southclaws
Copy link
Owner

Re-opening this because I realised Root is useless in the chain after I fix #23 since you can access the root error simply by reading the 0th element of the chain.

So, I'll probably just change Chain to:

type Chain []Step

@Southclaws Southclaws added enhancement New feature or request help wanted Extra attention is needed labels Nov 30, 2022
@Southclaws
Copy link
Owner

Resolved in v0.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants