fix: Update TwilioRestClient.cs to add overloaded method in rest client#770
fix: Update TwilioRestClient.cs to add overloaded method in rest client#770
Conversation
|
Can you add a description and see the test cases are failing. |
Added a description. All test cases are passing |
|
|
||
| AuthStrategy noauthstrategy = new NoAuthStrategy(); | ||
| _noAuthRestClient = new TwilioRestClient(_username, _password, accountSid: _accountSid, region: _region, edge: _edge, authstrategy: noauthstrategy) | ||
| _noAuthRestClient = new TwilioRestClient(_username, _password, authstrategy: noauthstrategy, accountSid: _accountSid, region: _region, edge: _edge) |
There was a problem hiding this comment.
Is it the case that order in which the params are passed, is considered? If that's the case, we should add authStrategy as the last param since it won't break any current user who is passing region or edge specifically.
There was a problem hiding this comment.
Order in which parameters passed is not considered if we mention the name of the parameters.
There was an issue reported because we added a new optional parameter auth strategy
In this PR, this was implemented as two different overloaded methods. Restored the old method as it was. Added a new method with authstrategy a mandatory parameter. If we add auth strategy as an optional parameter, these two method calls become ambigous and throws error while compiling
So I added auth strategy as mandatory parameter. Mandatory parameters has to be appeared before optional ones
There was a problem hiding this comment.
Okay that makes sense. Thanks!
There was a problem hiding this comment.
hey - this change in order absolutlely matters and you have broken your aspnet library with this change
this change is not binary compatible with the prior release and there is now MissingMethodExceptions occurring because of this.
Named parameters (and optional parameters) are only syntactic sugar. they are baked into the caller and the compiler will reorder and substitute defaults in the calling library.
You need to urgently release an updated version of the aspnet library that is compiled against this version of the package. Or preferably fix the constructor on this package to make it compatible again.
Also changing a public constructor on a public class is a breaking change, thats a semver major version, this could have been easily avoided by not changing the order.
Added a test case in a different PR |
|
4 participants
Fixes
https://twilio-engineering.atlassian.net/browse/DII-1756
A short description of what this PR does.
Adding overloaded method in TwilioRestClient with and without authstrategy.
Restoring the existing method in TwilioRestClient and adding a new overloaded method with a new mandatory parameter authStrategy
Checklist
If you have questions, please file a support ticket, or create a GitHub Issue in this repository.