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
Tests for UDA removal #70
Conversation
Hey! This is good stuff, @countermeasure. I'm supposed to be on vacation this weekend so I'll give it all a closer look next week. Maybe @coddingtonbear would like to give it a once over too? Adam, some backstory: Sky and I talked over email about adding some failing test cases for some features he would like to see and these are some of the patches that came out of that. |
Sure! I'm going to be on a camping trip for much of the weekend, but I think I'll have a little time. |
These two small commits seem to fix my problems, and from what I can tell they don't seem to cause any more tests to fail. The tests I wrote are still failing though, and it's because of this behaviour which I don't understand:
We create a task with only a description, then add a UDA value for The second last command, Here though, it causes the value None to be added back to the Any thoughts? |
It think handling
so Sound right? Hopefully no one ever needs to set |
@@ -6,7 +6,8 @@ | |||
class NumericField(Field): | |||
def serialize(self, value): | |||
if not isinstance(value, numbers.Number): | |||
raise ValueError("Value must be numeric.") | |||
if value: |
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.
Are users not allowed to set a numeric field to null
in taskwarrior?
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.
With task-2.4.0.beta3, if I modify the task with task SOME_ID mod MY_NUMERIC_UDA:
, it just removes it (and doesn't set it to null
).
Any ideas on this @coddingtonbear? You're the mastermind behind the marshalling code. :P I've been putting off looking at this, but if you don't have any ideas off the top of your head I can dive in and dig around. |
So many ideas, actually -- I'll have a look once I have a few minutes, but what you've described sounds approximately like what I vaguely remember writing, but I'll have to do a little more digging. |
OK -- I've found the root of this, and it's this chunk of code in def __getitem__(self, key):
try:
return super(Task, self).__getitem__(key)
except KeyError:
converter = self._fields.get(key, None)
if converter is None:
# If we don't have something registered as a converter,
# let's raise KeyError as we always would have.
raise
# Otherwise, let's return the empty value for this field.
# This is primarily helpful for fields that *might* be
# unspecified in the JSON, but are always part of a task
# record -- like annotations.
# Furthermore, since we're returning a value we don't really
# "have", we should stick it in the collection to be consistent
# for future queries.
value = self._deserialize(key, None)
super(Task, self).__setitem__(key, value)
return value Honestly, I can't remember the reason that I did the above, but I'd bet it's a workaround I put in place to make sure that accessing I don't think changing this will be a big deal, really, but it might take a little bit of work, and would be backwards-incompatible. That said, I'm not sure that this was wrapped up in a release yet, so I think I might be able to fix this without a major version bump? What do you think @ralphbean? |
I believe it was put out in the |
OK guys, I finally spent a little time to dig into this; sorry for the delay! I've pushed up some changes to a branch named Let me know if you bump into any questions, though! |
I pushed up commit cc1d78a just now to try and get some of this working with the old DirectDB approach. |
Hey! The only thing left broken in the test suite in the So, given that -- do you two feel like it's okay to merge that, pending removal of those two failing tests? |
+1 |
I've had a go at writing some failing tests for UDAs which have had a value set, then have the value removed.
There's a problem which seems to come up when
marshal=True
, and I've noticed it in particular for numeric UDAs.If I do this for a task which has a value of say
15
for a UDAmy_numeric_uda
:Then as far as I can tell the expected Taskwarrior behaviour for the following should be a KeyError:
The different types of UDAs seem to fail on this front for different reasons.
I might have a go at fixing this myself, but I'm a novice so I can't vouch for the quality of these tests, or any fixes I might come up with. Please review carefully before accepting!