Support due times #117

untitaker · 2017-02-22T10:54:47Z

dt_separator is necessary because of parsing due dates

WhyNotHugo

I really like this approach because we suppress the need to customize compact_detail, which is ugly. 👍

WhyNotHugo · 2017-02-22T12:21:47Z

todoman/confspec.ini

@@ -14,6 +14,13 @@ color = option('always', 'auto', 'never', default='auto')
 # this option is not specified the system locale's is used.
 date_format = string(default='%x')

+# The date format used both for displaying times, and parsing input times.
+# Default is `%H:%M`.


There's no need to clarify this default due to how the doc is rendered (the one from below will be picked up and shown already).

WhyNotHugo · 2017-02-22T12:22:51Z

todoman/ui.py

-            assert isinstance(date, datetime)
-            if date.date() in self.special_dates:
-                rv = self.special_dates[date.date()]
+            assert type(date) is datetime.date


We don't really need this assertion any more due to how we call this function (we've also made it sort of non-public now).

WhyNotHugo · 2017-02-22T12:23:01Z

todoman/ui.py

-            return None
+    def _format_time(self, time):
+        if time:
+            assert isinstance(time, datetime.time)


WhyNotHugo · 2017-02-22T12:29:19Z

todoman/confspec.ini

+time_format = string(default='%H:%M')
+
+# The string used to separate date and time when displaying and parsing.
+dt_separator = string(default=' ')


As a default, I'd prefer to have:

if time_format: return ' ' else: return ''

This way, we avoid an extra space when there's no time_format (I think it's burdensome for users to have to disable the separator when they don't use times.

If one of time_format or date_format is an empty string, dt_separator is not used. Hence the filter(bool, ...) construction.

WhyNotHugo · 2017-02-22T12:31:50Z

tests/test_formatter.py

-    assert formatter.format_date(any_day) == any_day.strftime(DATE_FORMAT)
+    assert formatter.format_datetime("") == "              "
+    assert formatter._format_date(today.date()) == "   Today"
+    assert formatter._format_date(tomorrow.date()) == "Tomorrow"


Can we also have a test for ' Tomorrow 12:00'?

WhyNotHugo · 2017-02-22T12:33:54Z

todoman/ui.py

+            date_part = None
+            time_part = None
+        else:
+            assert isinstance(dt, datetime.datetime)


Since we're doing this asserting, can't we do dt.strtime(self.datetime_format)? Can a datetime not have a time component?

To deal with special_dates, we can just substitute 'Tomorrow' with 'Tomorror' + SEPARATOR + TIME_FORMAT (at construction time).

Using dt.strftime obliterates the entire special-casing done in #108

Not the way I was thinking it:

self.special_dates = { self.now.date(): "today".rjust(self.date_width) + self.dt_separator + self.time_format,

Then, in format_datetime:

if dt.date() in self.special_dates rv = dt.strftime(self.special_dates[date.date()])

This would limit our capability to add special strings for times in the future ("in one hour" comes to mind)

"In one hour" is a datetime format, not a time format. Eg: you'd never see 'Tomorrow in one hour". So this would actually make that simpler.

I just noticed that for "in one hour" we'd need an if-else chain, not just a mapping. In either case it's updated now.

untitaker · 2017-02-22T18:49:29Z

@hobarrera I think I made all requested changes.

WhyNotHugo

There some bits I'm a bit unsure of, but I think it's mostly me getting confused.

WhyNotHugo · 2017-02-23T12:27:09Z

todoman/ui.py

+        # Map special dates to the special string we need to return
+        self.special_datetimes = {}
+        for key, value in mapping:
+            if isinstance(value, tuple):


Is this if ever false?

Fixed. Left-over when I was trying to add "in one hour" (but as said that would be only implementable as if-else).

WhyNotHugo · 2017-02-23T12:30:39Z

todoman/ui.py

+        if dt in self.special_datetimes:
+            format_string = self.special_datetimes[dt]
+        elif dt.date() in self.special_datetimes:
+            format_string = self.special_datetimes[dt.date()]


Won't this exclude the time?

No, as you asked me to do, the format strings now include the time as well. E.g. there's a value like "Today %H:%M" in that dictionary (assuming a particular time_format).

Oh, I completely misread this, yes, it makes sense.

I don't think the first bit of the if is neccessary though, since we never format mere dates.

WhyNotHugo · 2017-02-23T12:31:22Z

todoman/ui.py

-            return self.empty_date
+            format_string = self.datetime_format
+        if not dt:
+            return format_string


Should this be empty string?

That'd miss the " "-padding such that the columns are aligned properly. It is self.special_datetimes[None].

WhyNotHugo · 2017-02-23T12:32:23Z

todoman/ui.py

-                )
-            rv = datetime.fromtimestamp(mktime(rv))
-
+        rv = self._parse_datetime_naive(dt)


Maybe we can merge these functions now that they'd so much shorter?

_parse_datetime_naive has multiple return paths, those would all have to add the tzinfo. That's the only reason for the split. (BTW the parsing functions didn't change in my recent commits, they've been like that from the beginning)

untitaker · 2017-02-23T12:43:26Z

re ab7a995: Yeah, that's probably cleaner.

WhyNotHugo · 2017-02-23T12:44:51Z

re ab7a995: Yeah, that's probably cleaner.

Great, I just though it was faster to push it than explain it. 😄

untitaker · 2017-02-23T12:47:11Z

Ok i think this is ready to merge

WhyNotHugo · 2017-02-23T12:48:42Z

tests/test_formatter.py

-    assert formatter.format_date(tomorrow) == "Tomorrow"
-    assert formatter.format_date(any_day) == any_day.strftime(DATE_FORMAT)
+    assert formatter.format_datetime("") == "              "
+    assert "Today" in formatter.format_datetime(today.date())


I prefer the version with the string with spaces (eg: " Today"), because it also checks that we don't break column width.

WhyNotHugo · 2017-02-23T12:49:47Z

todoman/ui.py

@@ -39,13 +39,13 @@ def __init__(self, todo, lists, formatter):

        if todo.due:
            # TODO: use proper date_format


I think this TODO is old an obsolete?

untitaker · 2017-02-23T13:21:53Z

Since you don't appear to be on IRC:

I would rather revert back to the old variant. Eventually I want to add support for bare dates for DUE (without time component) and this preprocessing makes it very hard.

WhyNotHugo · 2017-02-23T14:14:34Z

Yeah, I jump in and out of IRC, TBH.

So, does any of the rest of the stack (rfc/icalendar/Todo/cache) support bare dates?

untitaker · 2017-02-23T14:18:39Z

No, it would all have to be refactored for that. My point is that this code will require more changes with your proposed implementation.

WhyNotHugo · 2017-02-23T14:34:32Z

You're right, this won't work formatting separately, sadly. :(

I wonder if we might just as well using something like arrow.humanize (code) to do the work for us.

WhyNotHugo · 2017-02-23T14:35:43Z

I've also had my eye on maya for some time now, and it might also be capable of doing some parsing (though we really should review it before changing anything).

untitaker · 2017-02-23T14:36:10Z

Possibly in the future, but that's a lot of code there. I think we should be more conservative about "humanizing" datetimes, preferrably without loss of precision

WhyNotHugo · 2017-02-23T14:36:49Z

Back to this PR though:

Yes, my proposal is bad.
We should also support things like --due 10:30. If the event has a date, only change the time. If it doesn't make it today at 10:30.

untitaker · 2017-02-23T14:39:30Z

I force-pushed the old variant.

untitaker · 2017-02-23T14:40:46Z

If the event has a date, only change the time.

This will be complicated. Right now only datetimes are passed around.

WhyNotHugo · 2017-02-23T14:50:23Z

Sure, let's keep this simple and discuss more improvements in a separate issue.

untitaker · 2017-02-23T14:50:47Z

I pushed a commit that allows the user to enter --due 12:00, and it will be parsed as "today 12am" (regardless of previous due value)

WhyNotHugo · 2017-02-23T14:51:02Z

If the event has a date, only change the time.

I meant, "if it has a datetime" (as opposed to having None).

untitaker · 2017-02-23T14:52:10Z

So to be clear, if I set todo.due to a time, you want that only the time component gets updated, not the entire value replaced? This sounds counterintuitive.

WhyNotHugo · 2017-02-23T14:53:29Z

My idea was that, basically, when a user sets a time (note: this is really pseuocode, not python), with something like todo edit 2 --due 10:30:

if todo.due:
   todo.due.set_time(time):
else:
   todo.due = datetime(today, time)

Does this seem too unintuitive (I'm biased because it's my idea)?

untitaker · 2017-02-23T14:54:22Z

In the CLI module todo properties are set in a much more generic way.

WhyNotHugo · 2017-02-23T14:56:01Z

Okay then, I won't distract any more, and let's aim to keep this simple. ^_^

WhyNotHugo

Please add a changelog entry, otherwise, LGTM 🚀

untitaker · 2017-02-23T15:22:38Z

I added a bit of validation to the config parsing code, please review again.

WhyNotHugo · 2017-02-23T16:41:59Z

todoman/configuration.py

@@ -83,4 +83,18 @@ def load_config():
                'Bad {} setting, {}'.format(key, error.args[0])
            )

+    date_format = config['main']['date_format']


I'd rather see new validators that include this in this block (eg: like validate_colour and validate_cache_path.
Mostly for separation of concerns, and to avoid this function growing too much.

WhyNotHugo · 2017-02-23T16:58:45Z

I pushed the changes I was suggesting for the validation.

untitaker · 2017-02-23T17:00:30Z

Oops, sorry I didn't notice that there are already ConfigObj validators, or that ConfigObj even had that capability.

WhyNotHugo · 2017-02-23T17:01:18Z

LGTM, but since I pushed the last change, I'll also wait for you thumbs-up.

untitaker · 2017-02-23T17:01:48Z

I would but I cannot approve my own PR

WhyNotHugo · 2017-02-23T17:03:25Z

Feel free to merge 🚀
(once CI passes).

untitaker · 2017-02-23T17:11:10Z

Travis triggers a push-type build, but that is nonsensical since the PR branch is outside of the main repo. Since this is a required status check I can't merge the PR

WhyNotHugo · 2017-02-23T17:18:57Z

I squash-pushed this, travis should stop being dumb (I think it got confused when I pushed to pimutils/time-support, but that's still a bug on their side.

untitaker mentioned this pull request Feb 22, 2017

Support times for due dates #109

Closed

WhyNotHugo requested changes Feb 22, 2017

View reviewed changes

WhyNotHugo reviewed Feb 23, 2017

View reviewed changes

WhyNotHugo requested changes Feb 23, 2017

View reviewed changes

untitaker force-pushed the time-support branch from afc8f76 to 9bd20dd Compare February 23, 2017 14:38

WhyNotHugo requested changes Feb 23, 2017

View reviewed changes

WhyNotHugo approved these changes Feb 23, 2017

View reviewed changes

Support due times

9022732

WhyNotHugo force-pushed the time-support branch from b21183c to 9022732 Compare February 23, 2017 17:18

untitaker merged commit 1bcdd38 into pimutils:master Feb 23, 2017

untitaker deleted the time-support branch February 23, 2017 17:26

		@@ -39,13 +39,13 @@ def __init__(self, todo, lists, formatter):

		if todo.due:
		# TODO: use proper date_format

Support due times #117

Support due times #117

Conversation

untitaker commented Feb 22, 2017

WhyNotHugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhyNotHugo Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Feb 22, 2017

WhyNotHugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017

untitaker commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017

untitaker commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017

untitaker commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017

untitaker commented Feb 23, 2017

untitaker commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017

untitaker commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017

untitaker commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017 • edited Loading

untitaker commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017

WhyNotHugo left a comment

Choose a reason for hiding this comment

untitaker commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhyNotHugo commented Feb 23, 2017

untitaker commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017

untitaker commented Feb 23, 2017 • edited Loading

WhyNotHugo commented Feb 23, 2017 • edited Loading

untitaker commented Feb 23, 2017

WhyNotHugo commented Feb 23, 2017

WhyNotHugo Feb 22, 2017 •

edited

Loading

WhyNotHugo commented Feb 23, 2017 •

edited

Loading

untitaker commented Feb 23, 2017 •

edited

Loading

WhyNotHugo commented Feb 23, 2017 •

edited

Loading