GothenburgBitFactory / tasklib

A Python library for interacting with taskwarrior databases.
http://tasklib.readthedocs.org/en/latest/
BSD 3-Clause "New" or "Revised" License
147 stars 28 forks source link

Store deserialized data in Task._data #19

Closed robgolding closed 9 years ago

robgolding commented 9 years ago

Addresses both #17 and #18 (and adds tests for those issues).

Instead of serializing data on the way in (via __setitem__) and deserializing on the way out (via __getitem__), this moves to storing deserialized (native) data in Task._data, and only serializing when we save. That means the following will be possible:

t = Task(description='test task')
t.save()
t['tags'].append('test tag')
t['dependencies'].add(dependency)
robgolding commented 9 years ago

adding/appending works, but not when the keys are currently empty. That's because we just do a deep copy of the _data dict and compare with that, but __getitem__ is still doing some extra work to deserialize None if the key wasn't present. I'll have to think about this one to come up with a fix :)

robgolding commented 9 years ago

Good idea on that workaround @tbabej, I figured we might as well do it for all attributes so no need to introduce _stores_mutable_type.

tbabej commented 9 years ago

Yeah, the reason I wanted to do it only for the mutable objects is the following:

Suppose we have a task with no priority set. Now, if you do:

print task['priority']
task.save()

The task will get saved with a command which looks like this:

task 1 modify priority:

Since 'priority' is now generated in _modified_fields. This is not a good outcome imho. Clearly this was not the intention of the user. It may get in a way with default values for priority, or hooks that try to set default values for other attributes.

NOTE: Doing this only for mutable objects, of course, does not remove this issue entirely (e.g. it would still happen with depends), but it only mitigates the issue.

We need to think how to circumvent this entirely.

robgolding commented 9 years ago

@tbabej so long as the tests pass I think this is good to go now. Can you review and if you're happy we'll merge!

coveralls commented 9 years ago

Coverage Status

Coverage increased (+2.8%) when pulling a2961748b5773cab010a6352710460a26b4a2cbe on deserialized-data-dict into 6855958d83f2a0b2580e68744df7cc5f9a967391 on develop.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+2.49%) when pulling 68aa8a1759d350a4e6fcd79ab6b2e0e61e7bd538 on deserialized-data-dict into 6855958d83f2a0b2580e68744df7cc5f9a967391 on develop.

tbabej commented 9 years ago

Yes, I went over the code and it looks fine. There's still the nitpick two comments above, but I have an idea how to solve that. Will send pull request in the evening.