search

whisperfish / rust-phonenumber

Library for parsing, formatting and validating international phone numbers.
Apache License 2.0
163 stars 56 forks source link

Rm `thiserror` dep #61

Closed bheylin closed 1 year ago

bheylin commented 1 year ago

Including thiserror in a lib forces the users of the lib to build thiserror and it's dependencies. Namely proc-macro2, quote and syn.

These dependencies will more than likely be included through another dependecy path, but given that there are only three Error types (one only for build.rs) and the expansion of the Error definitions are simple, it's better to drop thiserror in favor of keeping the dependecy set as small as possible.

rubdos commented 1 year ago

it's better to drop thiserror in favor of keeping the dependecy set as small as possible.

I disagree with this sentiment. thiserror is a de-facto standard (so are syn and proc-macro2) in the ecosystem, it keeps the code simpler and more legible, and it eases the developer experience of the crate.

I do agree that the error types are rather simple, but that does not account for future expansion of the library.

So since those three crates are most probably in your dependency tree anyway, since they don't take any runtime overhead, and since they help reduce burden on the developer, I don't think this is mergeable.