-
Notifications
You must be signed in to change notification settings - Fork 26
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
[CIVIS-1609] FIX no assert
statements in non-test code
#430
Conversation
@@ -17,13 +17,13 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |||
from `ContainerFuture` to `CivisFuture` (#426) | |||
- Updated the docstrings for `file_to_civis` (for `buf` and `expires_at`), | |||
`dataframe_to_file` (for `expires_at`), and `json_to_file` (for `expires_at`). (#427) | |||
- Added default values from swagger in client method's signature (#416) |
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.
Just cleaning up -- this is a duplicate entry for the same one a couple lines above
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.
LGMT! Thanks for giving me insight!
@@ -3,6 +3,9 @@ | |||
from civis._utils import camel_to_snake | |||
|
|||
|
|||
_RETURN_TYPES = frozenset({'snake', 'raw', 'pandas'}) |
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 are the snake and raw return types? I couldn't figure out what this is. Thanks Jackson!
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.
It's in the docstring of APICilent
:
Lines 352 to 361 in 6c7213e
return_type : str, optional | |
The following types are implemented: | |
- ``'raw'`` Returns the raw :class:`requests:requests.Response` object. | |
- ``'snake'`` Returns a :class:`civis.response.Response` object for the | |
json-encoded content of a response. This maps the top-level json | |
keys to snake_case. | |
- ``'pandas'`` Returns a :class:`pandas:pandas.DataFrame` for | |
list-like responses and a :class:`pandas:pandas.Series` for single a | |
json response. |
The default value in APIClient
is "snake"
, since in the Python world you'd expect the attributes to be in snake_case
. If you pick "raw"
(which I've never used myself in practice), you'd get attributes in camelCase
instead.
assert
statements shouldn't be used in the actual, non-test code, since they can be disabled by the-O
flag: https://docs.python.org/3/using/cmdline.html#cmdoption-O It's not clear how likely folks would run Civis-related code with this flag on, but it's better to convert theseassert
statements into conditional statements with exception raising.