Refactor Thrift.Transport.SSL.configuration/1 #517
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The implementation did not respect its type signature, which specified that the error form of the return value should contain an exception as the second element. This expectation is relied upon by maybe_ssl_handshake. However the callback passed in the :configure option could return anything, and we return its response without fully verifying that it matches the configuration typespec. Also refactored the code for clarity.
|
I must have closed #511 accidentally. |
fishcakez
approved these changes
Oct 20, 2020
lib/thrift/transport/ssl.ex
Outdated
| {:ok, opts} -> | ||
| handle_optional(opts) | ||
|
|
||
| {:error, %_exception{}} = error -> |
There was a problem hiding this comment.
This is a little defensive as elixir patterns go because the second tuple would always match given the rest of the code. Usually wed only be so defensive at the edge of an API this defensively, which youve done below from the callback result.
There was a problem hiding this comment.
Sure that's fine.
| @@ -43,26 +49,23 @@ defmodule Thrift.Transport.SSL do | |||
| {:ok, extra_opts} -> | |||
| {:ok, extra_opts ++ opts} | |||
|
|
|||
| {:error, _} = error -> | |||
| {:error, %_exception{}} = error -> | |||
There was a problem hiding this comment.
Perhaps Exception.exception? if we want to check this as an exception is more than struct.
There was a problem hiding this comment.
For now I've just matched the same pattern as in protocol_handler.ex and client.ex.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The implementation did not respect its type signature, which specified that the
error form of the return value should contain an exception as the second
element. This expectation is relied upon by maybe_ssl_handshake. However the
callback passed in the :configure option could return anything, and we return
its response without fully verifying that it matches the configuration
typespec.
Also refactored the code for clarity.