Initial implementation to address #102 and provide datetime objects#103
Initial implementation to address #102 and provide datetime objects#103graysonarts merged 5 commits intodevelopmenttableau/server-client-python:developmentfrom feature-102-datetimestableau/server-client-python:feature-102-datetimesCopy head branch name to clipboard
Conversation
|
I don't like exposing setters that users should not call. That doesn't feel good right now. Maybe one day we will have a reason to expose this but we don't right now. Our public api set should be clear. Also not sure how it is that you didn't have to change the parse code to use the setter. Did I miss something there? It feels like we should either use parse_datetime function when building the object from the response or we should have an "internal setter". |
|
@LGraber that was pretty much the same conclusion I was coming to, but I figured I'd throw out what I had to make sure others felt the same way. I only have unit tests written (this wasn't intended to be a complete PR) so the parsing code doesn't have tests yet, it was just where I got by the time I had to leave for meetings. I'll continue this tomorrow morning to finish it out. My approach now will be to call parse_datetime in the appropriate parsing functions where we parse out created_at and updated_at |
| return wrapper | ||
|
|
||
|
|
||
| def property_is_datetime(func): |
There was a problem hiding this comment.
Even though this isn't being used right now, I left it in. If you feel like I should delete it, I can.
LGraber
left a comment
There was a problem hiding this comment.
Looks good to me. I only had one question about the update to the travis file.
| - python setup.py test | ||
| # pep8 - disabled for now until we can scrub the files to make sure we pass before turning it on | ||
| - pycodestyle . | ||
| - pycodestyle tableauserverclient test |
There was a problem hiding this comment.
A recent update to pycodestyle (since we always install the latest in the travis run) is picking up files that are outside of our control (libraries installed), so now we want to limit to specifically code we have control over.
| return wrapper | ||
|
|
||
|
|
||
| def property_is_datetime(func): |
I'm a little conflicted on this implementation. created_at and updated_at are not user settable, but I went down the "use it as a property" route for coercing to the datetime object, so I wanted to get other opinions on it. The other option would be to change the _set_values functions to call the parse_datetime function instead of using the property.
Thoughts?