serverlesstechnology / cqrs

A lightweight, opinionated CQRS and event sourcing framework.
Other
370 stars 40 forks source link

use 'thiserror' to define 'Error' #39

Closed danieleades closed 1 year ago

davegarred commented 1 year ago

I don't know that this adds enough value to warrant pulling in an additional dependency. That not only adds to the library upkeep burden but can be off-putting to users.

Though likely fine with most users this change can be anywhere from annoying to those that prefer a different error library to particularly burdensome on those in companies where each dependency must be approved by security.

danieleades commented 1 year ago

annoying to those that prefer a different error library

thiserror is not exposed in the API, it's just a compile-time only helper which is precisely equivalent to the existing error implementation this library already has- it just replaces the boilerplate with a derive macro.

particularly burdensome on those in companies where each dependency must be approved by security.

This i understand, as this is something i have to manage in my day job. thiserror is small, well-maintained, extremely widely-used, maintained by a well-known author, etc. Not a high-risk dependency by any means. It's also so widely-used that any company that both uses Rust code and has to vet transitive dependencies of libraries they use is very likely to have already reviewed this library.

for reference - https://github.com/dtolnay/thiserror/network/dependents

it's also already in the dependency tree for

danieleades commented 1 year ago

@davegarred any movement on this? Does my previous comment answer your questions?

davegarred commented 1 year ago

Hi @danieleades, I think we could give it a try. I do need you to resolve the conflicts though.