-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add timeouts variables to configuration #25
Conversation
This needs tests, a CHANGELOG entry and README documentation, please. |
@@ -3,7 +3,7 @@ module TaxCloud #:nodoc: | |||
class Client < Savon::Client | |||
# Create a new client. | |||
def initialize | |||
super wsdl: TaxCloud::WSDL_URL | |||
super client_params.merge(wsdl: TaxCloud::WSDL_URL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WSDL setting should move into client_params
as well IMO. There's no need for special treatment. Client params could be something close to:
{ wsdl: TaxCloud::WSDL_URL }.merge(TaxCloud.configuration)
I believe TaxCloud.configuration is always defined, or at least should be.
FYI I updated minitest and what not, fixing the build in #26, rebase. |
@@ -20,8 +20,6 @@ load 'lib/tasks/tax_cloud.rake' | |||
load 'lib/tasks/tax_codes.rake' | |||
load 'lib/tasks/tax_code_groups.rake' | |||
|
|||
load 'vcr/tasks/vcr.rake' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had an exception during load the file about its missing. Not sure that the change is correct.
See my comments + squash the commits please. |
@dblock , done. Please, review |
The code looks great, the only thing is about documentation - what is the timeout in ? Seconds? Milliseconds? Can you please update with making that clear? Thanks. |
fixed |
Merging, thanks for your cooperation! |
Add timeouts variables to configuration
No description provided.