added support for schedules by shinchris · Pull Request #30 · tableau/server-client-python

shinchris · 2016-09-13T23:30:53Z

No description provided.

t8y8 · 2016-09-13T23:54:46Z

test/test_schedule.py

+        self.assertEqual("Weekly", all_schedules[1].frequency)
+        self.assertEqual("2016-09-18T06:00:00Z", all_schedules[1].next_run_at)
+
+    def test_get_empty(self):


Do we test this for every content type? I'm not sure it's a super valuable test but it's harmless.

Is it just testing that the Schedule.from_response doesn't choke on an empty input?

If that's the case the mocked request might be unnecessary.

This is minor though, I bet this follows the other test's and that's cool.

t8y8 · 2016-09-13T23:57:46Z

🚀

graysonarts · 2016-09-14T15:54:37Z

🚀

LGraber · 2016-09-14T16:38:02Z

tableauserverclient/models/schedule_item.py

+        self._next_run_at = None
+        self._priority = None
+        self._state = None
+        self._type = None


What is type. This doesn't feel like it should be just an arbitrary string

"Extract" or "Subscription" -- maybe more in the future

t8y8 · 2016-09-16T00:07:33Z

Whoa, this got even bigger :)

It'll take me some time to take a look at it, but I've got a long flight tomorrow...

t8y8 · 2016-09-16T20:07:17Z

samples/create_schedules.py

+parser.add_argument('--username', '-u', required=True, help='username to sign into server')
+parser.add_argument('--logging-level', '-l', choices=['debug', 'info', 'error'], default='error',
+                    help='desired logging level (set to error by default)')
+args = parser.parse_args()


The other samples probably need this as well -- but this should be in an if __name__ == '__main__' block so it only runs as a script, and so that we can check for no arguments and print the help.

Addressing the other samples can be a different Issue#

Fixed this in all samples

t8y8 · 2016-09-16T20:10:06Z

tableauserverclient/models/interval_item.py

+from .. import NAMESPACE
+
+
+class IntervalItem(object):


I don't have an alternate suggestion, so feel free to ignore, but is IntervalItem the best name we've got?

The nested classes are fine for the enum-y purpose you're using it for (man, in py3 we could use real enums) -- but we might be hitting a point where we just needs a constants or common module that holds all these so the actual class with logic doesn't get muddled down.

I think this is also the case for RequestOptions?

Ignored for now

t8y8 · 2016-09-16T20:10:44Z

tableauserverclient/models/interval_item.py

+
+        cls._validate_time(start_time)
+        cls._validate_time(end_time)
+        interval = [(interval_occurrence.lower(), str(interval_value))]


If this doesn't need to be mutable can just return a tuple here

Not sure why this is a list, but it's consistent so I left it

t8y8 · 2016-09-16T20:12:13Z

tableauserverclient/models/interval_item.py

+        self.start_time = start_time
+
+    @staticmethod
+    def _validate_time(t):


This might be useful just as a function at the module level, and returns true or false, then the classes that use it can decide what exception to throw, or to handle some other way (like using a default time)

I looked at it for a while and made a small refactor, but pulling it out can happen in another PR and after more conversation.

t8y8 · 2016-09-16T20:14:10Z

tableauserverclient/models/schedule_item.py

+            (_, name, _, _, updated_at, _, frequency, next_run_at, end_schedule_at, execution_order,
+             priority, interval_item) = self._parse_element(schedule_xml)
+
+            self._set_values(None, name, None, None, updated_at, None, frequency, next_run_at, end_schedule_at,


Can you use keyword arguments for all of these -- else we're doomed to be lost at sea :)

(Side note, can any of these be moved out of the function, it seems crazy big)

t8y8 · 2016-09-16T20:15:14Z

tableauserverclient/models/schedule_item.py

+
+    @classmethod
+    def from_response(cls, resp):
+        all_schedule_items = list()


Nit: We seem to use the literal in most places []

t8y8 · 2016-09-16T20:15:55Z

tableauserverclient/models/schedule_item.py

+             end_schedule_at, execution_order, priority, interval_item) = cls._parse_element(schedule_xml)
+
+            schedule_item = cls(name, priority, schedule_type, execution_order, interval_item)
+            schedule_item._set_values(id, None, state, created_at, updated_at, None, frequency, next_run_at,


Same comment about keyword arguments

t8y8 · 2016-09-16T20:16:47Z

tableauserverclient/models/schedule_item.py

+
+        priority = schedule_xml.get('priority', None)
+        if priority:
+            priority = int(priority)


This will fail if priority is a non empty string that isn't an integer. Do we know that's always safe to do this conversion?

t8y8 · 2016-09-16T20:22:53Z

tableauserverclient/server/endpoint/schedules_endpoint.py

+    def delete(self, schedule_id):
+        if not schedule_id:
+            error = "Schedule ID undefined"
+            raise ValueError(error)


The exception should get logged as an error -- or do we not do that on other endpoints?

Confirmed the XSD schema only allows nonNegInts

t8y8 · 2016-10-02T22:08:44Z

tableauserverclient/models/schedule_item.py

+
+        return self
+
+    def _set_values(self, id, name, state, created_at, updated_at, schedule_type, frequency,


@RussTheAerialist I took a local copy of this PR to try and address my own feedback and put it up for review, and this method made me wonder if there was a better way.

I'm thinking of instead of having a giant setter that does nothing but check that attributes are not false -- what if something like this:

The ScheduleItem constructor has everything initialized to None (or has default args)

_parse_element returns only non-null/None attributes it gets back as a dict

We can loop over the dict to apply the attributes in _set_values (and remove the giant function signature) or I think we can do away with the method entirely, but the idea was on the tip of my tongue and it left me.

I can dig in farther, but for now I've at least done this:

schedule_item._set_values(id=id, name=None, state=state, created_at=created_at, updated_at=updated_at, schedule_type=None, frequency=frequency, next_run_at=next_run_at, end_schedule_at=end_schedule_at, execution_order=None, priority=None, interval_item=None)

EDIT: I looked at the other *_item files and this is the pattern we use everywhere, I still think it's worth converting everything to keyword arguments though!

graysonarts · 2016-10-04T18:02:40Z

#48 supersedes this change

@shinchris

Original PR by @shinchris #30 * added ability to query and delete schedules * added ability to create and update schedules * intervals for schedules are expressed as (Unit)Interval classes * hourly intervals can take .25 and .5 to represent 15 and 30 minute schedules

added ability to query and delete schedules

03e8ce3

t8y8 reviewed Sep 13, 2016
View reviewed changes

LGraber reviewed Sep 14, 2016
View reviewed changes

added ability to create and update schedules

312a1c7

shinchris changed the title ~~added ability to query and delete schedules~~ added support for schedules Sep 16, 2016

t8y8 suggested changes Sep 16, 2016

View reviewed changes

t8y8 reviewed Oct 2, 2016

View reviewed changes

t8y8 added a commit to t8y8/server-client-python that referenced this pull request Oct 3, 2016

Resubmitting tableau#30 with a few changes.

0adb03f

t8y8 mentioned this pull request Oct 3, 2016

Adding Schedules Support Round 2 #48

Merged

graysonarts closed this Oct 4, 2016

graysonarts deleted the feature-schedule branch October 28, 2016 22:12


		return self

		def _set_values(self, id, name, state, created_at, updated_at, schedule_type, frequency,

Conversation

shinchris commented Sep 13, 2016

Uh oh!

t8y8 Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8y8 commented Sep 13, 2016

Uh oh!

graysonarts commented Sep 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8y8 commented Sep 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8y8 Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graysonarts commented Oct 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

t8y8 Sep 13, 2016 •

edited

Loading

t8y8 Oct 2, 2016 •

edited

Loading