ash-project / splode

Aggregatable and consistent errors for Elixir
MIT License
17 stars 4 forks source link

Added 'merge_with' option to 'use Splode' #8

Closed pinetops closed 5 months ago

pinetops commented 5 months ago

Per discussion started on Slack.

I wanted to review a few decisions:

  1. I didn't change splode_error?. The reasoning was that a) it's used in its current form in Ash so I wanted to be cautious about changing the behavior. b) the naming seems less than ideal given that it no longer answers the original question: new name is merge_error? And c) the function can pull both MODULE and the list of merge_with from the context without using arguments, that seemed to me to be a better interface.

  2. I've made some assumptions about the data structure:

    • That the top level class is always an ErrorClass, and that it contains a list of Errors which can themselves contain an ordinary error (like a RuntimeError). This is based on what I've seen in Ash, and it works well for my example.
    • I've implemented it recursively, so the error property can contain another error class, which can itself contain a list of errors. I'd be happy to be told that this can't happen :-). This is generalized such that something that the tree is traversed if something that contains error, itself contains something that contains errors
    • The actual implementation is slightly more general and basically assumes that something with %{error: ...} contains something with %{errors: ...} or it is a leaf node

But in general it would be helpful to know what the range of valid possibilities for the error tree, and if there are more I need to cover (or indeed less)

Contributor checklist

zachdaniel commented 5 months ago

I'll work up a branch to propose some changes. The goal isn't to have custom merging like this, but rather to leverage the existing to_class logic, which handles this kind of thing already. Such that if you have an error from one of these other splodes, to_error will leave it as is if its one of those other splode's errors, and to_class will incorporate that error into its own logic (i.e using its own error class to categorize it.)

zachdaniel commented 5 months ago

Also, we won't pull things out of their UnknownError, only their error classes, so the tests will need to adjust to use an unknown error class. Any individual error should be retained from the other error source.

zachdaniel commented 5 months ago

I've opened a PR here, to show what I had in mind. https://github.com/ash-project/splode/pull/8