added support for schedules #30
Conversation
| 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): |
There was a problem hiding this comment.
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.
|
🚀 |
1 similar comment
|
🚀 |
| self._next_run_at = None | ||
| self._priority = None | ||
| self._state = None | ||
| self._type = None |
There was a problem hiding this comment.
What is type. This doesn't feel like it should be just an arbitrary string
There was a problem hiding this comment.
"Extract" or "Subscription" -- maybe more in the future
shinchris
changed the title
|
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... |
| 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Addressing the other samples can be a different Issue#
| from .. import NAMESPACE | ||
|
|
||
|
|
||
| class IntervalItem(object): |
There was a problem hiding this comment.
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?
|
|
||
| cls._validate_time(start_time) | ||
| cls._validate_time(end_time) | ||
| interval = [(interval_occurrence.lower(), str(interval_value))] |
There was a problem hiding this comment.
If this doesn't need to be mutable can just return a tuple here
There was a problem hiding this comment.
Not sure why this is a list, but it's consistent so I left it
| self.start_time = start_time | ||
|
|
||
| @staticmethod | ||
| def _validate_time(t): |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| (_, 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, |
There was a problem hiding this comment.
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)
|
|
||
| @classmethod | ||
| def from_response(cls, resp): | ||
| all_schedule_items = list() |
There was a problem hiding this comment.
Nit: We seem to use the literal in most places []
| 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, |
There was a problem hiding this comment.
Same comment about keyword arguments
|
|
||
| priority = schedule_xml.get('priority', None) | ||
| if priority: | ||
| priority = int(priority) |
There was a problem hiding this comment.
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?
| def delete(self, schedule_id): | ||
| if not schedule_id: | ||
| error = "Schedule ID undefined" | ||
| raise ValueError(error) |
There was a problem hiding this comment.
The exception should get logged as an error -- or do we not do that on other endpoints?
There was a problem hiding this comment.
Confirmed the XSD schema only allows nonNegInts
|
|
||
| return self | ||
|
|
||
| def _set_values(self, id, name, state, created_at, updated_at, schedule_type, frequency, |
There was a problem hiding this comment.
@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!
|
#48 supersedes this change |
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
No description provided.