-
Notifications
You must be signed in to change notification settings - Fork 46
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
base: develop
Are you sure you want to change the base?
Make 'depends' field a little more tolerant of malformed data #155
Conversation
from .base import DirtyableList, Field | ||
|
||
|
||
class DependsField(Field): |
There was a problem hiding this comment.
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.
return DirtyableList([]) | ||
|
||
try: | ||
# In task-2.5, this moved from a comma-separated string to a real list. |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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.
Let's hold off on reviewing this, actually -- I did a little more research as part of https://github.com/coddingtonbear/python-taskwarrior and discovered a couple things:
I'm not clear if the corrupted data will make that not work, though! |
On Inthe.AM, I very regularly encounter malformed values for
depends
. I'm not even slightly sure what kind of bug is causing this, but I'm fairly sure it's between a taskwarrior client and the taskserver itself, and thus the fix is a little outside our wheelhouse here. That being said: I recognize that this kind of strategy is controversial, so I welcome discussion about this approach.What this does is, should an incoming value for
depends
not be deserializable, attempt to extract all findable UUIDs in the field using a regular expression.