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

Make 'depends' field a little more tolerant of malformed data #155

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion taskw/fields/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from .base import Field
from .annotationarray import AnnotationArrayField
from .array import ArrayField
from .commaseparateduuid import CommaSeparatedUUIDField
from .depends import DependsField
from .choice import ChoiceField
from .date import DateField
from .duration import DurationField
Expand Down
39 changes: 0 additions & 39 deletions taskw/fields/commaseparateduuid.py

This file was deleted.

49 changes: 49 additions & 0 deletions taskw/fields/depends.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
from __future__ import absolute_import

from distutils.version import LooseVersion

import re
import uuid

from .base import DirtyableList, Field


class DependsField(Field):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed this field since the name was potentially misleading, and given that we long ago made this field specific to the idiosyncrasies of 'depends', it's probably reasonable to name it to indicate that.

version = LooseVersion('2.4')

def deserialize(self, value):
if not value:
return DirtyableList([])

try:
# In task-2.5, this moved from a comma-separated string to a real list.
Copy link
Collaborator Author

@coddingtonbear coddingtonbear Jan 23, 2022

Choose a reason for hiding this comment

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

I can't find any evidence of this and I wonder if this is actually referencing the on-disk format, possibly?

Looking at the docs, the value of this field is indeed a string: https://taskwarrior.org/docs/design/task.html#attr_depends

That all being said -- the weirdness I see on Inthe.AM is that there's a JSON-serialized list stored in this text field 🤷.

Nevermind -- the linked doc there clearly says that in the future this might be converted to return a list.

# here we allow a migration path where old splitable strings are
# handled as well as newschool lists.
value_list = value
if hasattr(value_list, "split"): # It's not actually a list!
value_list = value_list.split(",")

return DirtyableList([uuid.UUID(v) for v in value_list])
except ValueError:
dependency_finder = re.compile(
"[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}"
)
return DirtyableList(
[uuid.UUID(v) for v in dependency_finder.findall(value)]
)

def serialize(self, value):
if not value:
value = []

if not hasattr(value, "__iter__"):
raise ValueError("Value must be list or tuple, not %r." % value)

if self.version < LooseVersion("2.5"):
return ",".join([str(v) for v in value])
else:
# We never hit this second code branch now. taskwarrior changed
# API slightly in version 2.5, but we're just going to go with
Copy link
Collaborator Author

@coddingtonbear coddingtonbear Jan 23, 2022

Choose a reason for hiding this comment

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

Also don't think this is true, either! See https://taskwarrior.org/docs/design/task.html#attr_depends.

Actually, looking at the field, they do specifically say that "in the future" this will be changed to be a list:

Note that in a future version of this specification, this will be changed to a JSON array of strings, like the "tags" field.

All I can say is that it's clearly not a list in at least 2.5.3 locally for myself.

# backwards compatibility for now.
# Some day we should switch wholesale to the new path.
return [str(v) for v in value]
4 changes: 2 additions & 2 deletions taskw/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
AnnotationArrayField,
ArrayField,
ChoiceField,
CommaSeparatedUUIDField,
DependsField,
DateField,
DurationField,
Field,
Expand All @@ -29,7 +29,7 @@
class Task(dict):
FIELDS = {
'annotations': AnnotationArrayField(label='Annotations'),
'depends': CommaSeparatedUUIDField(label='Depends Upon'),
'depends': DependsField(label='Depends Upon'),
'description': StringField(label='Description'),
'due': DateField(label='Due'),
'end': DateField(label='Ended'),
Expand Down
25 changes: 23 additions & 2 deletions taskw/test/test_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ def test_serialize_case_insensitive(self):
self.assertEqual(actual_value, expected_value)


class TestCommaSeparatedUUIDField(TestCase):
class TestDependsField(TestCase):
def setUp(self):
self.field = fields.CommaSeparatedUUIDField()
self.field = fields.DependsField()

def test_serialize_single_uuid(self):
single_uuid = [uuid.uuid4()]
Expand Down Expand Up @@ -218,6 +218,27 @@ def test_deserialize_uuid_string_undashed(self):

self.assertEqual(actual_value, expected_value)

def test_deserialize_malformed_1(self):
malformed_depends_field = "[\"eb3a14ce-fee7-4309-b097-aca4ac21851d\",\"ee4ff9af-9017-4292-a4cd-afc363802e4d\"]"

actual_value = self.field.deserialize(malformed_depends_field)
expected_value = [
uuid.UUID("eb3a14ce-fee7-4309-b097-aca4ac21851d"),
uuid.UUID("ee4ff9af-9017-4292-a4cd-afc363802e4d"),
]

self.assertEqual(actual_value, expected_value)

def test_deserialize_malformed_2(self):
malformed_depends_field = "[\"[\"59aec08a-a9cc-4804-8e98-9abdd2f8595e\"\",\"\"e3a32703-45e6-436b-8274-6f39b46f2ded\"]\"]"

actual_value = self.field.deserialize(malformed_depends_field)
expected_value = [
uuid.UUID("59aec08a-a9cc-4804-8e98-9abdd2f8595e"),
uuid.UUID("e3a32703-45e6-436b-8274-6f39b46f2ded"),
]

self.assertEqual(actual_value, expected_value)

class TestDateField(TestCase):
def setUp(self):
Expand Down