Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

initial checkin for get background jobs#298

Merged
graysonarts merged 3 commits intotableau:developmenttableau/server-client-python:developmentfrom
graysonarts:add-get-all-jobsCopy head branch name to clipboard
May 31, 2018
Merged

initial checkin for get background jobs#298
graysonarts merged 3 commits intotableau:developmenttableau/server-client-python:developmentfrom
graysonarts:add-get-all-jobsCopy head branch name to clipboard

Conversation

@graysonarts
Copy link
Contributor

New Endpoint from the scheduling team. First pass for the first endpoint

@graysonarts graysonarts requested review from shinchris and t8y8 May 29, 2018 22:30
Copy link
Collaborator

@t8y8 t8y8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 with some ?'s/nits


@property
def name(self):
"""For APi consistency"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring can probably go, or expand on what "API consistency" means

def _safe_to_log(server_response):
'''Checks if the server_response content is not xml (eg binary image or zip)
"""Checks if the server_response content is not xml (eg binary image or zip)
and and replaces it with a constant
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're tweaking my comment, can you fix my double 'and and' while you're there :)

import warnings
warnings.warn("use use_server_version instead", DeprecationWarning)

def assert_at_least_version(self, version):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why move this to the server object itself?

import warnings
warnings.warn("Jobs.get(job_id) is deprecated, update code to use Jobs.get_by_id(job_id)")
return self.get_by_id(job_id)
self.parent_srv.assert_at_least_version('3.1')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now, you need to call this assert to check that it's > 3.1 and also keep the > 2.6 for old back compat. I need to think about this, maybe it's better to have another decorator behavior_changed_in... or something like that.

But this seems fine for now

Copy link
Contributor

@shinchris shinchris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 with some nits

id_ = element.get('id', None)
type_ = element.get('jobType', None)
status = element.get('status', None)
created_at = element.get('createdAt', None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add datetime parsing for all of the time fields using parse_datetime.py?

test/test_job.py Outdated
self.assertEquals('Success', job.status)
self.assertEquals('50', job.priority)
self.assertEquals('single_subscription_notify', job.type)
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add asserts for the 3 datetime fields that are part of the xml response?

Also wondering if the xml response is left here intentionally as a comment.

@t8y8
Copy link
Collaborator

t8y8 commented May 31, 2018

🚀

@graysonarts graysonarts merged commit 6ddbb80 into tableau:development May 31, 2018
@graysonarts graysonarts deleted the add-get-all-jobs branch May 31, 2018 15:32
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.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.