Add newSecureClientConnection and friends#46
Conversation
Addresses the closed issue tfausak#22.
|
Thanks for opening this PR! Sorry I haven't been able to review it yet. I will try to take a close look soon. From a quick glance, it looks good but I wonder if anything can be combined/de-duplicated. |
|
True, the |
tfausak
left a comment
There was a problem hiding this comment.
Looks great!
I think having the runSecureClient* functions call each other makes for a cleaner implementation than delegating to runSecureClientConnection*, since we can avoid a bunch of calls to bracket.
On the other hand, it means we're duplicating the default options/config. Not a huge deal, but a potential for drift.
I figured I'd leave my suggestions, but I'm happy to merge this as-is. I don't think either approach is strictly better.
|
I don't have strong feelings either way either, on balance I thought the brackets were boilerplatey but easy to verify aren't doing anything weird and won't need to be modified, whereas the default config code was (slightly) more likely to change over time and drift as you say, but happy with whatever the maintainer prefers. |
Co-authored-by: Taylor Fausak <taylor@fausak.me>
Co-authored-by: Taylor Fausak <taylor@fausak.me>
tfausak
left a comment
There was a problem hiding this comment.
Looks great, thanks! Sorry for the back and forth.
Addresses the closed issue #22.