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.

Sign up for GitHub

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

Jump to bottom

Properly parse empty priority in the UI #170

Merged
merged 1 commit into from
Mar 4, 2017
Merged

Properly parse empty priority in the UI #170

merged 1 commit into from
Mar 4, 2017

Conversation

WhyNotHugo
Copy link
Member

Fixes #156

@WhyNotHugo
Copy link
Member Author

This contains the first real UI test. 🎉

@WhyNotHugo
Copy link
Member Author

Moving forward, I think it makes sense to have a test_formatter fixture which just returns TodoFormatter with the DATE_FORMAT and TIME_FORMAT we're using for tests.

@WhyNotHugo
Copy link
Member Author

@untitaker I hope you don't take reviewing this as an imposition, I just feel you'd like to review this (mostly because it sets a new example for tests) and you might have some comments or suggestions (or not).

untitaker
untitaker requested changes Mar 4, 2017
View reviewed changes
Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make a difference between None and 0? They have the same semantics.

tests/conftest.py Outdated
@@ -69,6 +69,16 @@ def inner(tz='CET'):
return inner


@pytest.fixture
def blank_todo(default_database):
todo = model.FileTodo(new=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FileTodos are new by default.

tests/conftest.py Outdated
@@ -69,6 +69,16 @@ def inner(tz='CET'):
return inner


@pytest.fixture
def blank_todo(default_database):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more generally usable if this fixture returns a function that creates todos (eventually with a parameter for specifying list)

untitaker
untitaker requested changes Mar 4, 2017
View reviewed changes
Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reintroduces the bug fixed by #160 because now we might again end up using humanized datetimes when prefilling TodoEditor.

@untitaker untitaker mentioned this pull request Mar 4, 2017
Open
@WhyNotHugo
Copy link
Member Author

Why make a difference between None and 0? They have the same semantics.

Basically one is "don't store anything in the file", and the other "store 0".
I think this is a bit important, because if we edit a file, I'd like to avoid adding PRIORITY:0\n to it if the user didn't touch that field.

@WhyNotHugo
Copy link
Member Author

This reintroduces the bug fixed by #160 because now we might again end up using humanized datetimes when prefilling TodoEditor.

Are you sure this comment was meant for this PR?

@untitaker
Copy link
Member

Are you sure this comment was meant for this PR?

That was a mistake I think

@untitaker
Copy link
Member

It's up to you whether you want to make the model more complex for a nicer file diff.

@WhyNotHugo WhyNotHugo merged commit 0ef01ec into master Mar 4, 2017
@WhyNotHugo WhyNotHugo deleted the empty-priority-ui branch March 4, 2017 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@untitaker untitaker untitaker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@WhyNotHugo @untitaker