-
Notifications
You must be signed in to change notification settings - Fork 35
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
Params validation for API methods #119
Conversation
optimizely/helpers/validator.py
Outdated
@@ -151,3 +152,25 @@ def is_user_profile_valid(user_profile): | |||
return False | |||
|
|||
return True | |||
|
|||
|
|||
def is_non_empty_string(test_var): |
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.
nit. test_var
is not a good variable name.
optimizely/helpers/validator.py
Outdated
""" | ||
PY3 = sys.version_info[0] == 3 | ||
|
||
if PY3: |
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.
What happens if you don't do this?
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.
Namerror. basestring was removed in Python3. https://docs.python.org/3.0/whatsnew/3.0.html
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 am wondering if there is a better way to accomplish what you are doing here.
optimizely/helpers/validator.py
Outdated
Returns: | ||
Boolean depending upon whether input is valid or not. | ||
""" | ||
if isinstance(input, string_types) and input: |
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.
input is a built in Python function name. Please change this name.
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.
LGTM. Please address comments before merging.
optimizely/helpers/validator.py
Outdated
@@ -14,6 +14,7 @@ | |||
import json | |||
import jsonschema | |||
|
|||
from six import string_types |
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.
nit. This is an external package so it should be imported on line 16 and line 17 should be empty.
optimizely/helpers/validator.py
Outdated
Returns: | ||
Boolean depending upon whether input is valid or not. | ||
""" | ||
if isinstance(input_id_key, string_types) and input_id_key: |
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.
Perhaps flip it to be:
if input_id_key and isinstance(input_id_key, string_types)
self.assertFalse(validator.is_non_empty_string(1.2)) | ||
self.assertFalse(validator.is_non_empty_string(True)) | ||
self.assertFalse(validator.is_non_empty_string(False)) | ||
self.assertFalse(validator.is_non_empty_string("")) |
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.
nit. Single quotes
self.assertFalse(validator.is_non_empty_string(False)) | ||
self.assertFalse(validator.is_non_empty_string("")) | ||
|
||
self.assertTrue(validator.is_non_empty_string("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.
nit. Single quotes.
self.assertFalse(validator.is_non_empty_string("")) | ||
|
||
self.assertTrue(validator.is_non_empty_string("0")) | ||
self.assertTrue(validator.is_non_empty_string("test_user")) |
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.
Same as above.
travis ci is getting failed on Python 3.5, let us investigate this issue. |
@oakbani can you rebase against master and re-submit. Travis tests should pass then. |
No description provided.