Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic todo template #456

Merged
merged 17 commits into from Jun 29, 2018
Merged

Dynamic todo template #456

merged 17 commits into from Jun 29, 2018

Conversation

paopow
Copy link
Contributor

@paopow paopow commented Jun 28, 2018

Add an option to specify todos to remove or skip from a todo list template based on computed conditional properties.

@@ -592,6 +592,9 @@ class TodoListTemplate (TodoListTemplateMixin, BaseModel):
if it is generated by the system.
todos (str)
A JSON blob that describe the todos in the todo list template.
conditional_property_function (str)
A JSON blob containing the path to and name of a python method
that will return the preconditions to prune the created todos
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: period at end of sentence

todo.save()
for template_todo_item in template_todo.get('items', []):
_add_template_todo(template_todo_item, todolist_template, todo, task)
def _to_exclude(props, conditions=[]):
Copy link
Member

@marcua marcua Jun 28, 2018

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/5712904/empty-dictionary-as-default-value-for-keyword-argument-in-python-function-dicti :)

you can do this:

def _to_exclude(props, conditions=None):
    for condition in conditions or []:

Copy link
Member

Choose a reason for hiding this comment

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

alternatively, you already pass in [] below, so just make conditions required ;)

get_cond_props = locate(path)
cond_props = get_cond_props(task.project)
except Exception:
logger.exception('Invalid condition function path.')
Copy link
Member

Choose a reason for hiding this comment

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

condition->contional

@@ -592,6 +592,9 @@ class TodoListTemplate (TodoListTemplateMixin, BaseModel):
if it is generated by the system.
todos (str)
A JSON blob that describe the todos in the todo list template.
conditional_property_function (str)
Copy link
Member

Choose a reason for hiding this comment

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

explain the structure---is it a list of conditions that get ORed together, with properties in each dictionary getting ANDed, or the other way?

todo.save()
for template_todo_item in template_todo.get('items', []):
_add_template_todo(template_todo_item, todolist_template, todo, task)
def _to_exclude(props, conditions=[]):
Copy link
Member

Choose a reason for hiding this comment

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

docstring to rehash this question above:

explain the structure---is it a list of conditions that get ORed together, with properties in each dictionary getting ANDed, or the other way?

current_value = props.get(prop)
compared_to_value = predicate['value']
if predicate['operator'] == '=':
return current_value == compared_to_value
Copy link
Member

Choose a reason for hiding this comment

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

since these return immediately, they only ever check the first property of the first condition. you probably want to keep a boolean around that you keep flipping depending on properties/conditions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another way to implement string operators -> https://stackoverflow.com/questions/44554614/eval-string-cmp-operator

This might be useful if we intend to extend the available operators in future.

Copy link
Member

@marcua marcua left a comment

Choose a reason for hiding this comment

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

A few things

Copy link
Collaborator

@adbharadwaj adbharadwaj left a comment

Choose a reason for hiding this comment

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

A small suggestion regarding the string operators and a comment on skip propagation.

current_value = props.get(prop)
compared_to_value = predicate['value']
if predicate['operator'] == '=':
return current_value == compared_to_value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another way to implement string operators -> https://stackoverflow.com/questions/44554614/eval-string-cmp-operator

This might be useful if we intend to extend the available operators in future.

description=template_todo['description'],
template=todolist_template,
parent_todo=parent_todo,
skipped_datetime=skipped_datetime
Copy link
Collaborator

Choose a reason for hiding this comment

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

To propogate skipping from parents to descendents.

skipped_datetime = timezone.now() if parent_todo.skipped_datetime or to_skip else None

We may even want to skip evaluation of to_skip if parent_todo.skipped_datetime is not Null. That may save considerable computation time.

@coveralls
Copy link

coveralls commented Jun 29, 2018

Coverage Status

Coverage increased (+0.04%) to 93.801% when pulling 4e2ccb1 on dynamic_todo_template into 5eadce1 on master.

def _to_exclude(props, conditions):
"""
The conditions is it a list of conditions that get ORed together,
with properties in each dictionary getting ANDed.
Copy link
Member

Choose a reason for hiding this comment

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

with predicates in each dictionary

for condition in conditions:
all_props_true = True
Copy link
Member

Choose a reason for hiding this comment

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

are we fine with a zero-item list of properties evaluating to true? could do all_props_true = True and len(all_props_true) > 0

"""
any_condition_true = False

for condition in conditions:
all_props_true = True
all_props_true = len(condition) > 0
Copy link
Member

Choose a reason for hiding this comment

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

len(conditions) :P

Copy link
Contributor Author

@paopow paopow Jun 29, 2018

Choose a reason for hiding this comment

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

This is for detecting {} (a condition). if len(conditions) == 0, this line wouldn't get executed.

template=todolist_template
)
root_todo.save()
with transaction.atomic():
Copy link
Member

@marcua marcua Jun 29, 2018

Choose a reason for hiding this comment

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

you could wrap the entire function in @transaction.atomic and then you don't have to indent everything :)

value (number, bool, sring, or None):
The value to compare with of the predicate.
"""
operator = jsl.StringField(required=True, pattern='[!=><]=|[><]')
Copy link
Member

Choose a reason for hiding this comment

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

[!=><]=|[><] looks like a serious Kirby battle

Copy link
Member

@marcua marcua left a comment

Choose a reason for hiding this comment

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

[!=><]=|[><]

@paopow paopow merged commit bc7c00c into master Jun 29, 2018
@paopow paopow deleted the dynamic_todo_template branch June 29, 2018 19:00
@paopow paopow restored the dynamic_todo_template branch June 29, 2018 19:27
paopow added a commit that referenced this pull request Jun 29, 2018
paopow added a commit that referenced this pull request Jun 29, 2018
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.

None yet

4 participants