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

Tests for UDA removal #70

Merged
merged 4 commits into from Dec 11, 2014
Merged

Tests for UDA removal #70

merged 4 commits into from Dec 11, 2014

Conversation

countermeasure
Copy link
Contributor

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 UDA my_numeric_uda:

task['my_numeric_uda'] = None
id, task = w.task_update(task)

Then as far as I can tell the expected Taskwarrior behaviour for the following should be a KeyError:

task['my_numeric_uda']

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!

@ralphbean
Copy link
Owner

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.

@coddingtonbear coddingtonbear self-assigned this Oct 10, 2014
@coddingtonbear
Copy link
Collaborator

Sure! I'm going to be on a camping trip for much of the weekend, but I think I'll have a little time.

@countermeasure
Copy link
Contributor Author

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:

>>> from taskw import TaskWarrior
>>> w = TaskWarrior(marshal=True)
>>> task = w.task_add("Simple task")
>>> task
{u'status': u'pending', u'description': u'Simple task', u'entry': datetime.datetime(2014, 10, 11, 6, 2, 6, tzinfo=tzutc()), u'id': 31, u'urgency': 0, u'uuid': UUID('d5337c40-2952-482d-a51c-6ecbbb4c7f6a')}
>>> task['time'] = 30
>>> id, task = w.task_update(task)
>>> task
{u'status': u'pending', u'description': u'Simple task', u'modified': datetime.datetime(2014, 10, 11, 6, 2, 51, tzinfo=tzutc()), u'time': 30, u'entry': datetime.datetime(2014, 10, 11, 6, 2, 6, tzinfo=tzutc()), u'id': 31, u'urgency': 0, u'uuid': UUID('d5337c40-2952-482d-a51c-6ecbbb4c7f6a')}
>>> task['time'] = None
>>> task
{u'status': u'pending', u'description': u'Simple task', u'modified': datetime.datetime(2014, 10, 11, 6, 2, 51, tzinfo=tzutc()), u'time': None, u'entry': datetime.datetime(2014, 10, 11, 6, 2, 6, tzinfo=tzutc()), u'id': 31, u'urgency': 0, u'uuid': UUID('d5337c40-2952-482d-a51c-6ecbbb4c7f6a')}
>>> id, task = w.task_update(task)
>>> task
{u'status': u'pending', u'uuid': UUID('d5337c40-2952-482d-a51c-6ecbbb4c7f6a'), u'entry': datetime.datetime(2014, 10, 11, 6, 2, 6, tzinfo=tzutc()), u'description': u'Simple task', u'modified': datetime.datetime(2014, 10, 11, 6, 3, 27, tzinfo=tzutc()), u'urgency': 0, u'id': 31}
>>> task['time']
>>> task
{u'status': u'pending', u'uuid': UUID('d5337c40-2952-482d-a51c-6ecbbb4c7f6a'), 'time': None, u'entry': datetime.datetime(2014, 10, 11, 6, 2, 6, tzinfo=tzutc()), u'description': u'Simple task', u'modified': datetime.datetime(2014, 10, 11, 6, 3, 27, tzinfo=tzutc()), u'urgency': 0, u'id': 31}

We create a task with only a description, then add a UDA value for time, then remove the UDA value.

The second last command, task['time'] should cause a KeyError, and it does when marshal=False.

Here though, it causes the value None to be added back to the time UDA for the task.

Any thoughts?

@countermeasure
Copy link
Contributor Author

It think handling 0 will be okay because

>>> isinstance(0, numbers.Number)
True

so 0 shouldn't cause us to enter the if statement.

Sound right?

Hopefully no one ever needs to set awesomeness to 0 though!

@@ -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:
Copy link
Collaborator

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?

Copy link
Owner

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).

@ralphbean
Copy link
Owner

We create a task with only a description, then add a UDA value for time, then remove the UDA value.

The second last command, task['time'] should cause a KeyError, and it does when marshal=False.

Here though, it causes the value None to be added back to the time UDA for the task.

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.

@coddingtonbear
Copy link
Collaborator

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.

@coddingtonbear
Copy link
Collaborator

OK -- I've found the root of this, and it's this chunk of code in taskw.task:

    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 annotations or other list fields always returned an array (albeit an empty one).

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?

@ralphbean
Copy link
Owner

I'm not sure that this was wrapped up in a release yet

I believe it was put out in the 0.8.x releases, but marshal defaulted to False to give us time to shake out kinks like these. I'd be fine with changing it for a 0.9.x line. :)

@coddingtonbear
Copy link
Collaborator

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 uda_handling_alterations that should remove some unnecessarily confusing behaviors. I think that that'll solve the problem for you, @countermeasure.

Let me know if you bump into any questions, though!

@ralphbean
Copy link
Owner

I pushed up commit cc1d78a just now to try and get some of this working with the old DirectDB approach.

@ralphbean
Copy link
Owner

Hey! The only thing left broken in the test suite in the uda_handling_alterations branch is support for numeric UDAs when marshal=False.. and I don't think it is possible to handle numeric udas without it.

So, given that -- do you two feel like it's okay to merge that, pending removal of those two failing tests?

@coddingtonbear
Copy link
Collaborator

+1

@coddingtonbear coddingtonbear removed their assignment Dec 11, 2014
@ralphbean ralphbean merged commit e040459 into ralphbean:develop Dec 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants