-
-
Notifications
You must be signed in to change notification settings - Fork 308
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
Refactor of settings.py #142
Refactor of settings.py #142
Conversation
|
||
return cert | ||
return cert or None # TODO: Should change rtype docstrings or update tests |
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.
Can the docstring be updated? Not sure if changing the return type to be only string is a good idea at this stage.
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 don't think is a good idea, let's keep return types (and only change that if strictly necessary)
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 updated the docstrings and removed the TODO comments.
Code looks more pythonic, great job! |
I implemented that toolkit from the php-saml toolkit and I willfully tried to keep the structure between the toolkits (same with the java and dotnet) in order to help me in the global maintenance. But I think this specific change that you proposed does not affect me to port future changes on the rest of the toolkits. I will review and merge. Thanks for contribute! (I will apply those changes also on python3-saml |
@pitbulk you've done a terrific job maintaining this library in many languages, this PR was just to make it more readable to those used to Python conventions, and I agree the structure should be kept as much as possible (e.g. although in Python getters and setters are not common practice, I'd leave them there). |
Thanks @Brachi , I read fast your comment and saw the"terrific" word that alarmed me. This is a spanish "false friend". Thanks for contribute! Sure we can find a balance between portability and python code style :) |
I tried to apply this change to python3-saml and the tests failed. I will need to review if we changed the behavior: https://github.com/onelogin/python3-saml/pull/24/files |
@pitbulk it seems tests/src/OneLogin/saml2_tests/response_test.py is failing at setup time in both master and the settings_refactor branches. If that test file is removed all other tests pass (including settings_test). Is that the failure you had? |
never mind, the problem was in the tests :) |
Changed several parts of the code to reduce verbosity and make it a little more readable. No functionality changed. Added 2 TODOs to discuss functions that return a string or None, to either update the docstrings or the functions + the tests