-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
Added the possibility to use local WSDL files #11
Conversation
The recommended way to install this library is [through composer](packagist://getcomposer.org). | ||
[New to composer?](packagist://getcomposer.org/doc/00-intro.md) | ||
The recommended way to install this library is [through composer](http://getcomposer.org). | ||
[New to composer?](http://getcomposer.org/doc/00-intro.md) |
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 did not do this manually. I don't really know what happened here (?!)
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.
Looks like your feature branch is based on an old version of the master branch. Can you rebase and push your changes?
Awesome, I like the new feature! However, I'm not sure about the method names |
I don't know. I can only agree with you on the fact that createClientFromWsdl is not the smartest name you can give to that method. Possible candidates :
What do you prefer ? Do you have any other idea ? |
ebedc0f
to
7a5c5a8
Compare
IMO the original Also, once we aim for #5, we should make it more obvious which method uses WSDLs. This means we should aim to rename the original method name (in a separate PR!). Your new method does in fact return a Can you update your PR with the new method name? 👍 Also, I think we should emphasize the difference to the existing method (documentation). |
I don't know if renaming the original function makes sense. This creates a compatibility break which you might want to avoid. The fact that createClient does not return a client is not a problem to me. It says that it's creating a client, not that it's returning it. I'm kind of biased though, I'm used to working with promises for everything :) If you're ready to do it anyway, which I can understand, I would rename it to something like createFromWsdlUrl. It'd make it clearer that it's loading a remote WSDL file, which to me implies that it's returning a promise. I'd rename the createClientFromWsdl method I'm writing to createClientFromDefinition make it clear that the method creates a client using a preloaded definition file (WSDL). The fact that createClient does not return a client is not a problem to me. It says that it's creating a client, not that it's returning it. I'm kind of biased though, I'm used to working with promises for everything :) I can't say I particularly like createClientWsdl, because, it might let people think that it generates a WSDL file, which would be confusing IMO. It's all up to you ! |
You're raising some very valid points here 👍 You're also right in that it probably doesn't make much sense to discuss this in this PR :) As such, I've just filed #15, so let's address these there 👍 Let's get back on topic here :) In fact, I like the Would you care to update the the documentation as to how this new method differs from the existing, dreaded |
It's already added in commit 7a5c5a8. It is OK for you ? |
Much appreciated, thanks @floriansimon1! 👍 |
Added the possibility to use local WSDL files
The previous pull request contained bad commits. This one should be better.