Skip to content
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

Some refactoring for future fixes #12

Merged
merged 4 commits into from Jul 25, 2014

Conversation

anton-ryzhov
Copy link
Contributor

Hello guys! I'd chosen your library as best available for python and I've started to use it in my project.
I've faced with some issues, and going to fix it and make pull-requests.
As first step I've made some refactoring — today wsdl_parse is too complex to understand and work with.
Looks like I've made no degradation with this fix according to tests.

I hope you'll like it and further fixes.

@reingart
Copy link
Member

Thanks Anton!
I'm in the middle of a Google Summer of Code project so I don't have much time to review this.
If you think this is convenient, I'll merge it, but please it would be wise if you can take a look to ensure you're not introducing future issues (for example, overwriting internal instance attributes like self.soap_ns, that change look suspicious, and it can be confusing as it is already a __soap_ns private instance attribute).

@anton-ryzhov
Copy link
Contributor Author

Good luck at GSC.

I would be more confident if unittests passed before and after changes) Now I only can compare errors before and after changes and ensure I haven't brought new.
I've double checked name collisions, but your remark is significant. I've renamed this property.

If you feel uncertainty about this fixes right now maybe we should wait until you'll have enough time.
I had a serious reason to rewrite entire _xml_tree_to_services on the next step (I'm preparing new PR). This definitely fixed critical bug for me, but there I'm not sure I've done everything correct. I'll need your to check it.

@reingart
Copy link
Member

Yes, the unittests needs care, many webservices are no more available and others simply fails because it cannot download the WSDL attached to each ticket in googlecode.
I started to clean up the test but I hit some other serious issues to me, so I had to pospone it.
If you could finish the test's clean up so Travis CI don't keep failing, that would be great.

About you uncertain feelling, I prefer taking the risk of merging your PR if no serious regression is introduced.
I've to review the wsdl parsing code for my own projects too, to I'd prefer doing it with you changes commited, that will be more productive and your refactory will make easier to fix the outstanding issues.

Thanks again!

@anton-ryzhov
Copy link
Contributor Author

Okey. Merge this now, I'll introduce my rewritten version based on this PR and we'll work on it to fit library to ours requirements.

Test fixes — sorry, I don't have enough time for it right now.

reingart added a commit that referenced this pull request Jul 25, 2014
Some refactoring for future fixes
@reingart reingart merged commit c573e2e into pysimplesoap:master Jul 25, 2014
@anton-ryzhov anton-ryzhov deleted the refactoring branch July 25, 2014 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants