-
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
Use field read_only
attribute to determine which fields should be included in task_update
call.
#156
base: develop
Are you sure you want to change the base?
Use field read_only
attribute to determine which fields should be included in task_update
call.
#156
Conversation
When modifying a task in taskwarrior 2.5.2 it was returning this error: The 'urgency' attribute does not allow a value of '0'. The solution is to drop the urgency argument if the value is 0 when calling task modify.
…ld include changes.
|
||
# Also, experimental mode returns the id. So, avoid comparing. | ||
del tasks['pending'][0]['id'] | ||
calculated_fields = ['urgency', 'id', 'modified'] |
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 debated whether to use field metadata here, too, and eventually landed on not using it so the test doesn't need to be aware of where field metadata is stored.
@@ -768,17 +768,15 @@ def task_update(self, task): | |||
else: | |||
task_uuid = task['uuid'] | |||
|
|||
id, original_task = self.get_task(uuid=task_uuid) | |||
|
|||
if 'id' in task: |
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.
This was actually a subtle bug -- we were mutating the passed-in task
object instead of its copy (which we'll generate a few lines below).
@@ -204,11 +204,12 @@ def set(self, key, value): | |||
""" Set a key's value regardless of whether a change is seen.""" | |||
return self.__setitem__(key, value, force=True) | |||
|
|||
def serialized(self): | |||
def serialized(self, include_immutable=True): |
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.
This isn't technically directly related to these changes, but shows a slight difference in approach in my first implementation of this. It's useful, though, in general for a side project and is backward-compatible, so I hope you'll forgive me for leaving this in.
Note that this build will fail until we merge #157. In the unlikely event that we can't agree to merge that PR, I'll update this one to not use the particular 3.6+ feature I'm using. |
We've historically had a list of fields we'd carefully remember to pop off of our task object before performing an update operation. We've also historically had a list of fields including whether those fields are
read_only
or not for use during serialization/deserialization of data. Let's just use that metadata for determining which fields are mutable and which aren't when performing atask_update
operation.