-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 "servers" support to the operation, path in PHP API client #2072
Conversation
@@ -54,19 +54,47 @@ use {{invokerPackage}}\ObjectSerializer; | |||
*/ | |||
protected $headerSelector; | |||
|
|||
/** | |||
* @var Host index |
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.
A 1st keyword of @var
identifies the type of the property. so I think it would be @var int Host index
.
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.
👍 I'll update it in the next push
*/ | ||
public function __construct( | ||
ClientInterface $client = null, | ||
Configuration $config = null, | ||
HeaderSelector $selector = null | ||
HeaderSelector $selector = null, | ||
$host_index = 0 |
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.
How about the $host_index
generates only if servers
exists in pathItems?
I just wondering that the $host_index
is defined but unused. 👀
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.
Yes, very good suggestion. My intention is to make the constructor consistent across different API classes.
What about updating the doc string to explain it cleary that it's optional and only needed when the endpoint has multiple servers/hosts defined?
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.
I agree with that. 👍
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.
Pushed some enhancements based on your feedback.
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.
Thanks for the update. LGTM!
…PITools#2072) * add servers support to path, operation in php client * remove servers from spec * update based on feedback
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
.master
,. Default:3.4.x
,4.0.x
master
.Description of the PR
An example of the change can be found in 64bf0d4#diff-15582b49a62d7a5ecac56414cfe6407fR332
cc @jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ackintosh (2017/09), @ybelenko (2018/07), @renepardon (2018/12)