Cycle Detection #160
Cycle Detection #160
Conversation
…ably bug fixes to cycle detection after)
…topological_sort()
| """ Checks for the existence of a method or list of methods """ | ||
| if isinstance(functions, str): | ||
| functions = [functions] |
There was a problem hiding this comment.
I figure to make this method general, I would accept either a string, or list of strings. This line converts the single string to a list with just one string, otherwise the for expected_method in functions: will loop through each char in the string.
There is no case currently that needs a list of functions (unless we want to get really explicit) so if you'd rather just simplify it to just have one string, I can change it to that.
There was a problem hiding this comment.
The function is definitely simpler (and harder for people in the future to screw up) if it's always operating on one type. Since we don't need the list functionality right now, I think we should just make it always accept a single string. This is also pretty easy to compose together later on if something needs to check multiple:
all(map(partial(self._implements_function, obj), ['function1', 'function2']))Could either do it that way, which I realize looks terrible to anyone who's not a LISP nerd, or write another function at the same level of _implements_function which explicitly operates on lists and calls the single function version to do its work.
|
I also just added a commit to make test_core.py pass PEP8 and pyflakes, so you'll see some whitespace in there |
| @@ -101,14 +104,14 @@ def test_dagobah_add_tasks(): | |||
|
|
|||
|
|
|||
| @with_setup(blank_dagobah) | |||
| def test_dagobah_delete_job(): | |||
| def test_dagobah_delete_job2(): | |||
| dagobah.add_job('test_job') | |||
There was a problem hiding this comment.
I renamed these two because they have the same name as an existing test, but are slightly different in how they test. Not sure if one is better than the other. But pyflakes was bitching.
| # Traverse nodes and verify any JobTasks | ||
| logger.debug("Traversing topologically sorted tasks.") | ||
| for task in topo_sorted: | ||
| if self.implements_expandable(task): |
There was a problem hiding this comment.
I want to change this to the negative check, and do a continue in that case, which will eliminate 1 level of indentation from the rest of the following code
ex:
if not self.implements_expandable(task):
continue
logger.debug("Found expandable task: {0}".format(task.name))
cur_job = self.parent._resolve_job(task.target_job_name)
... 
|
Done with first pass review, looks pretty good |
_implements_function originally took either a string, or list of strings. It has been decided that multiple datatypes are unnecessary, and end users can easily call the method on multiple strings in a list themselves. This commit also includes a deletion of an unnecessary log line.
This commit switches a conditional statement to the opposite in order to reduce the indentation on the following code block, which is sufficiently large enough to argue neatness. Also, an error is raised when referencing an invalid JobTask, instead of a warning with verfify() returning false. This is because in this case, the data is fundamentally broken.
|
Direct concerns with the previous PR have all been addressed. |
|
LGTM 🐻 |
This pull request is to add the
verify()method to theJobclass, which verifies that there are no cycles in the job, include cross-job cycles.Included are a few unit tests, including one really complex job where I tried to combine every possible way of linking jobs, verify them, and then add a loop at the end and make sure it can detect a loop deep in.
If you think of any edge cases you want me to cover, let me know!