Closed BapRx closed 1 year ago
This looks good! Just one inline comment about the wording, but apart from that, it's ready to be merged.
I will link issue #28 here because this PR shows the limitations of me using String
as the error type everywhere. In other projects, I started doing "proper" error handling using Error enum
s, which is much more ergonomic. It would also make the unit testing easier. You would not have to match against the exact String, but could just check against the enum variant. Turning the error into a user-facing string would happen as high on the stack as possible. </random stream of conciousness>
Anyway, thank you for your (second!) contribution!
(Same note about the CONTRIBUTORS
file as in your other PR applies).
Alright I updated the warning message and added my name to the CONTRIBUTORS :smile:
Merged, thanks for the contribution! :)
Here's what we discussed in #52, currently the behavior is to skip the remote if the type cannot be detected. That results in possibly empty remote in the config. I don't know if that is an issue or not.