Use tomorrow and today while formatting date #108

rimshaakhan · 2017-02-19T20:19:02Z

fixes #82

untitaker

In principle this looks good to me, still have a few nitpicks.

Also you have a few styleguide violations. After running pip install tox, you can get all style nitpicks using tox -eflake8.

EDIT: The output is also available here: https://travis-ci.org/pimutils/todoman/jobs/203232655#L249

untitaker · 2017-02-19T20:29:33Z

todoman/ui.py

        if date:
-            rv = date.strftime(self.date_format)
+            date_tomorrow = datetime.today().date() + timedelta(days = 1)


Instead of calling datetime.today() here, please use self.now. This is because we might want to replace self.now with a fixed value in the testsuite.

untitaker · 2017-02-19T20:30:43Z

todoman/ui.py

+            date_tomorrow = datetime.today().date() + timedelta(days = 1)
+            # Get a dictionary which stores the dates for which we don't have to merely
+            # return a date string and map it to the special string we need to return
+            dates_for_which_not_to_return_simple_date = {


This is quite a long variable name, I suggest something like special_dates.

untitaker · 2017-02-19T20:32:17Z

todoman/ui.py

+                datetime.today().date() : "Today",
+                date_tomorrow : "Tomorrow" ,
+            }
+            if date in dates_for_which_not_to_return_simple_date.keys():


.keys() is not necessary, just if date in dates_for_which_not_to_return_simple_date.

rimshaakhan · 2017-02-19T20:43:18Z

Thanks @untitaker for your kind help!

Looking forward to correct my mistakes

rimshaakhan · 2017-02-19T21:07:19Z

Hello @untitaker, do I need to write a test case for testing this feature?

untitaker · 2017-02-19T21:12:03Z

todoman/ui.py

+                self.now.date(): "Today",
+                date_tomorrow: "Tomorrow",
+            }
+            if date in special_dates():


You need to write special_dates instead of special_dates().

Oops! Sorry

No need to apologize!

untitaker · 2017-02-19T21:15:51Z

do I need to write a test case for testing this feature?

That would be nice. You have two options:

Write a test that simulates running todo, it would belong into https://github.com/pimutils/todoman/blob/master/tests/test_basic.py

For that option I imagine it would be enough to run todo new --due tomorrow hello and check if todo list outputs Tomorrow at some point. Also a similar testcase for Today.
Unit tests for the formatter. We currently have none, so this would require creating a new file tests/test_formatter.py. In the test you could create an instance of TodoFormatter (with a datetime format string of your choice), and test format_date with some input

WhyNotHugo · 2017-02-19T22:05:30Z

Hi. Thanks for picking this up with be being a bit away, @untitaker.

Another detail is that, if you run todo, the outputs are aligned, so today and yesterday are probably need to have an exact width. This is why no-date returns empty_date, which is a string with spaces to fill up the column.

You can probably do something like "today".rjust(WIDTH). I'd do this is the constructor (__init__) so we only compute this once (this function is called once per todo).

WhyNotHugo · 2017-02-20T01:03:56Z

It resolves Issue #82

Hint: if the message says something like "closes #82" or "fixes #82", merging the PR will automatically close the issue. ;)

rimshaakhan · 2017-02-20T06:05:49Z

The line which is making flake8 violations is here:

return special_dates[date] if date in special_dates else date.strftime(self.date_format)

Isn't it the optimal way of doing it or should I switch back to the if-else tree?

untitaker · 2017-02-20T11:40:24Z

@rimshaakhan The line is too long. I personally prefer the if-else variant.

WhyNotHugo

This looks quite on the right track, but there's some tiny optimizations I'd like to see.

We're a bit picky with code being very well polished, so don't feel unmotivated by all the back and forth. 😄

Again, feel free to ask any questions you may have.

WhyNotHugo · 2017-02-20T12:16:21Z

todoman/ui.py

+            # Get a dictionary which stores the dates for which
+            # we don't have to merely return a date string and
+            # map it to the special string we need to return
+            WIDTH = len(date.strftime(self.date_format))


Actually, len(empty_date) was fine. They should both have the same with, and we avoid formatting this date when it's [possibly] unnecessary.

I'd also prefer move this over to __init__ , so we only calculate this width once, instead every time we format a date (pretty much like we only calculate empty_date once.

Let me know if you're not very familiar with contructors/__init__ or if this is otherwise unclear.

Also, date_width is actually a better variable name, but that one's my fault because I used WIDTH on my example, which really was a bad idea on my part, sorry.

I guess you would want something like this:

def __init__(self, date_format): self.date_format = date_format self._localtimezone = tzlocal() self.now = datetime.now().replace(tzinfo=self._localtimezone) # An empty date which should be used in case no date is present self.empty_date = " " * len(self.format_date(self.now)) self.date_width = len(self.empty_date) self.date_tomorrow = self.now.date() + timedelta(days=1) # Get a dictionary which stores the dates for which # we don't have to merely return a date string and # map it to the special string we need to return self.special_dates = { self.now.date(): "Today".rjust(self.date_width, " "), self.date_tomorrow: "Tomorrow".rjust(self.date_width, " "), } self._parsedatetime_calendar = parsedatetime.Calendar()

But then wouldn't self.empty_date call self.format_date, where self.date_width and self.special_dates still won't exist and there would be an exception.
Should we find a different way for calculating self.empty_date?

Ugh, you're completely right, I missed that! I think we need to rewrite how we calculate empty_date and avoid that trap:

empty_date = self.now.format(self.date_format)

I don't think we need the comment explaining what that dictionary is; it seems quite clear. Maybe just a short one-liner, at most.

Also, since self.empty_date calls self.format_date(self.now)), it would not be an empty_date, rather something like " Today" (ideally - when we can get the code to run) and I think it would hinder the utility of having an empty_date and therefore would render it useless! :(

Oops (again!). Not by best day. I meant this:

self.date_width = len(self.now.format(date_format)) self.empty_date = " " * self.date_width

WhyNotHugo · 2017-02-20T12:16:23Z

todoman/ui.py

+            # map it to the special string we need to return
+            WIDTH = len(date.strftime(self.date_format))
+            special_dates = {
+                self.now.date(): "Today".rjust(WIDTH, " "),


Maybe the same as above; define the strings in the constructor.

rimshaakhan · 2017-02-20T13:48:00Z

No, I am not feeling demotivated, but on the contrary it gets even more exciting and adventurous.
It's feeling fun and on a serious note: I need to learn all these stuff, when else could have been a better time? - I am really grateful for having such technically insightful and creative critique on my work!
Thank You

untitaker

This looks good, I think the only thing missing are:

tests (see above, I suspect option 2 may be easier)
changelog entry (probably)
Adding yourself to AUTHORS

the requests in this review have been addressed

WhyNotHugo · 2017-02-20T18:05:16Z

FWIW, these final items are listed in the patch review checklist.

WhyNotHugo

LGTM so far, just authorship and tests 🎉

WhyNotHugo · 2017-02-20T18:06:55Z

todoman/ui.py

@@ -204,8 +204,16 @@ def __init__(self, date_format):
        self.date_format = date_format
        self._localtimezone = tzlocal()
        self.now = datetime.now().replace(tzinfo=self._localtimezone)
-        self.empty_date = " " * len(self.format_date(self.now))
-
+        self.date_tomorrow = self.now.date() + timedelta(days=1)


Just call this self.tomorrow, for consistency.

rimshaakhan · 2017-02-20T19:11:17Z

What should be added to changelog?

Perhpas, just a line saying: * Proper date formatting enabled would be enough!

untitaker · 2017-02-20T19:14:45Z

I would rather describe the concrete changes to todoman's behavior. The PR title is pretty close to that, though the word formatting may be too technical for the changelog, which is supposed to be a summary targeted at users.

untitaker

In #109 @hobarrera noticed that this PR has a bug

untitaker · 2017-02-20T19:57:03Z

todoman/ui.py

@@ -251,8 +259,18 @@ def detailed(self, todo):
        return rv

    def format_date(self, date):
+        """
+        :param datetime.date date: a datetime object


In fact format_date recieves a datetime object (i.e. datetime.datetime instead of datetime.date). It is probably useful to add an assert isinstance(date, datetime) in format_date.

untitaker · 2017-02-20T19:58:47Z

todoman/ui.py

        if date:
-            rv = date.strftime(self.date_format)
+            if date in self.special_dates:


Since date is a datetime (I know, misleading variable name), you need to use date.date() here, otherwise special_dates is never used.

Yes, sorry, this is something we've dragged for a while. #109 will fix this in future, but for now, just update this to date.date().

untitaker · 2017-02-20T19:58:59Z

tests/test_formatter.py

+    Tests the format_date function in todoman.ui.TodoFormatter
+    """
+    formatter = ui.TodoFormatter(DATE_FORMAT)
+    today = datetime.now().date()


Please make today a datetime instead of date

untitaker · 2017-02-20T20:01:35Z

@rimshaakhan if you run tox -epy locally, you will see that your current testcase fails. Also there are a few styling problems with your latest commits (see tox -eflake8)

rimshaakhan · 2017-02-21T21:18:11Z

The color checking test:

@pytest.mark.parametrize('hours', [72, -72])
def test_color_due_dates(tmpdir, runner, create, hours):
    due = datetime.datetime.now() + datetime.timedelta(hours=hours)
    create(
        'test.ics',
        'SUMMARY:aaa\n'
        'STATUS:IN-PROGRESS\n'
        'DUE;VALUE=DATE-TIME;TZID=ART:{}\n'
        .format(due.strftime('%Y%m%dT%H%M%S'))
    )

    result = runner.invoke(cli, ['--color', 'always'])
    assert not result.exception
    due_str = due.strftime('%Y-%m-%d')
    if hours == 1:
        assert result.output == \
            '  1 [ ]   {} aaa @default\x1b[0m\n'.format(due_str)
    else:
        assert result.output == \
            '  1 [ ]   \x1b[31m{}\x1b[0m aaa @default\x1b[0m\n'.format(due_str)

is failing with the error message:

            '  1 [ ]   {} aaa @default\x1b[0m\n'.format(due_str)
    else:
>       assert result.output == \
            '  1 [ ]   \x1b[31m{}\x1b[0m aaa @default\x1b[0m\n'.format(due_str)
E       assert '  1 [ ]   20...ault\x1b[0m\n' == '  1 [ ]   \x1...ault\x1b[0m\n'
E         -   1 [ ]   2017-02-25 aaa @default
E         +   1 [ ]   2017-02-25 aaa @default
E         ?           +++++          ++++

tests/test_basic.py:353: AssertionError

Can someone please share any insight @hobarrera @untitaker ?
Thanks!

untitaker · 2017-02-21T21:22:25Z

Yes, in that test there is a line called if hours == 1, it also needs to be changed to if hours == 72

WhyNotHugo

I think it's that tiny change and we're done. 🎉

WhyNotHugo · 2017-02-21T21:50:33Z

CHANGELOG.rst

@@ -23,6 +23,7 @@ v2.1.0
 * Fix crash when running ``--porcelain show``.
 * Show ``id`` for todos everywhere (eg: including new, etc).
 * Add the ``ctrl-s`` shortcut for saving in the interactive editor.
+* Show "Today" or "Tomorrow" when due date is today or tomorrow


v2.1.0 was released a couple of days ago, so you need to move this up. ^_^

Note that I try and keep things in reverse-chronological order too, so it should be at the very top.

WhyNotHugo · 2017-02-21T21:53:22Z

todoman/ui.py

@@ -251,8 +259,19 @@ def detailed(self, todo):
        return rv

    def format_date(self, date):
+        """
+        :param datetime.datetime date: a datetime object


:param: need to be at the bottom of the docstring, not at the top.

untitaker

Which editor do you use @rimshaakhan? There are probably plugins which immediately tell you about styleguide violations using flake8.

untitaker · 2017-02-21T21:56:11Z

tests/test_formatter.py

+    formatter = ui.TodoFormatter(DATE_FORMAT)
+    today = datetime.now()
+    tomorrow = today + timedelta(days = 1)
+    any_day = today + timedelta(days = randrange(2, 8))


So close! A styleguide violation here, no spaces around the =. Same for the previous line.

Hi @untitaker, I am using Sublime Text 3
It was so close! :)

Back when I used sublime I used a plugin called Anaconda that did some python style checks and stuff (don't recall what else). Might be worth looking into.

untitaker · 2017-02-21T22:15:30Z

I think this review comment is still pending, but then that'd be really it :)

rimshaakhan · 2017-02-21T22:15:59Z

Also, should I consider filing a issue because I am unable to filter out results when I run flake8 , it reports a issues in the env files.

Please have a look at the log file

WhyNotHugo · 2017-02-21T22:18:10Z

Also, should I consider filing a issue because I am unable to filter out results when I run flake8 , it reports a issues in the env files.

~~Run tox -e flake8 to only check todoman's files.~~

Scrub that, that'll also fail.

rimshaakhan · 2017-02-21T22:23:08Z

Run tox -e flake8 to only check todoman files.

Yeah! that was the command I ran, but didn't quite well worked for me!
Perhaps, it is because I have named the virtualenv as todoman_virtualenv (I may sound like a complete idiot )

WhyNotHugo · 2017-02-21T22:24:24Z

No, it's not really anything you've done. Personally, I never came across that because I use mkvirtualenv (which creates all the virtualenvs in a separate location). I'm not sure how other projects deal with that though.

WhyNotHugo · 2017-02-21T22:28:49Z

Thanks for you contribution! 🎉
Hope this has been useful and a worthwhile learning experience!

untitaker · 2017-02-21T22:29:35Z

In general I would recommend creating the virtualenv outside of the git repository, otherwise you may accidentally commit it into git, or misconfigured tools may crawl it like pytest or flake8.

rimshaakhan · 2017-02-21T22:34:44Z

@hobarrera You just beat me up while I was writing this Thank You :)
@untitaker @hobarrera Thank You sir for being so wonderfully patient and for giving me critical reviews.

I am looking forward to apply for RGSoC and am very excited to learn: how to make fascinating code which looks so amazingly beautiful!

Its a wonderful feeling of seeing myself in the author list
Thank You

untitaker · 2017-02-21T22:36:16Z

@rimshaakhan "beat me up" doesn't mean what you think it does! :D It has been a pleasure working with you so far!

rimshaakhan · 2017-02-21T22:40:01Z

In general I would recommend creating the virtualenv outside of the git repository, otherwise you may accidentally commit it into git, or misconfigured tools may crawl it like pytest or flake8.

@untitaker Should I re-configure for the same!
I did thought of it earlier, but couldn't think of a possible solution! (There's mkvirtualenv now :) as @hobarrera pointed out)

Use tomorrow and today while formatting date

8794ece

untitaker reviewed Feb 19, 2017

View reviewed changes

Rename varables and correct code according to syle guidelines

1aba597

untitaker reviewed Feb 19, 2017

View reviewed changes

Resolve a typo

1d66046

rimshaakhan force-pushed the master branch from 289f7fd to deef87d Compare February 20, 2017 05:21

WhyNotHugo previously requested changes Feb 20, 2017

View reviewed changes

rimshaakhan added 2 commits February 20, 2017 20:22

Added default values to constructor for ToDoFormatter

b10e3db

Removed unnecessary comments

07c67ac

untitaker requested changes Feb 20, 2017

View reviewed changes

WhyNotHugo requested changes Feb 20, 2017

View reviewed changes

WhyNotHugo mentioned this pull request Feb 20, 2017

Support times for due dates #109

Closed

rimshaakhan added 3 commits February 21, 2017 00:24

Replace date_tomorrow with tomorrow

ee2800b

Add test for TodoFormatter.format_date

21b223d

Add Rimsha Khan

92bc4e6

untitaker requested changes Feb 20, 2017

View reviewed changes

Justify strings and optimise format_date

deef87d

rimshaakhan added 4 commits February 22, 2017 02:34

Change test to be compliant with test_formtter

ad6a130

Resolve empty testcase issue

5eacecf

Merge branch 'master' of https://github.com/pimutils/todoman

4f25e7d

Remove print statement

b74b0a8

rimshaakhan added 2 commits February 22, 2017 03:17

fix test to be compliant with test_formatter

1c0282a

Add showing today/tomorrow as a feature

8cbb37a

WhyNotHugo requested changes Feb 21, 2017

View reviewed changes

untitaker approved these changes Feb 21, 2017

View reviewed changes

WhyNotHugo requested changes Feb 21, 2017

View reviewed changes

untitaker requested changes Feb 21, 2017

View reviewed changes

rimshaakhan added 2 commits February 22, 2017 03:31

Remove spaces from around arguements

749c727

Replace the param line to be at bottom

24adc9f

untitaker approved these changes Feb 21, 2017

View reviewed changes

Update changelogs for v2.2.0

c52384f

WhyNotHugo approved these changes Feb 21, 2017

View reviewed changes

WhyNotHugo merged commit 7d79f67 into pimutils:master Feb 21, 2017

untitaker mentioned this pull request Feb 22, 2017

Support due times #117

Merged

untitaker mentioned this pull request Mar 2, 2017

Implement --humanize #161

Merged

3 tasks

Use tomorrow and today while formatting date #108

Use tomorrow and today while formatting date #108

Conversation

rimshaakhan commented Feb 19, 2017 • edited Loading

untitaker left a comment • 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

rimshaakhan commented Feb 19, 2017

rimshaakhan commented Feb 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Feb 19, 2017 • edited Loading

WhyNotHugo commented Feb 19, 2017

WhyNotHugo commented Feb 20, 2017

rimshaakhan commented Feb 20, 2017 • edited Loading

untitaker commented Feb 20, 2017

WhyNotHugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rimshaakhan Feb 20, 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

Choose a reason for hiding this comment

rimshaakhan commented Feb 20, 2017

untitaker left a comment

Choose a reason for hiding this comment

WhyNotHugo commented Feb 20, 2017

WhyNotHugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rimshaakhan commented Feb 20, 2017

untitaker commented Feb 20, 2017

untitaker left a comment • 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 Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

untitaker commented Feb 20, 2017

rimshaakhan commented Feb 21, 2017 • edited Loading

untitaker commented Feb 21, 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

untitaker 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

untitaker commented Feb 21, 2017

rimshaakhan commented Feb 21, 2017

WhyNotHugo commented Feb 21, 2017 • edited Loading

rimshaakhan commented Feb 21, 2017

WhyNotHugo commented Feb 21, 2017

WhyNotHugo commented Feb 21, 2017

untitaker commented Feb 21, 2017

rimshaakhan commented Feb 21, 2017

untitaker commented Feb 21, 2017

rimshaakhan commented Feb 21, 2017

rimshaakhan commented Feb 19, 2017 •

edited

Loading

untitaker left a comment •

edited

Loading

untitaker commented Feb 19, 2017 •

edited

Loading

rimshaakhan commented Feb 20, 2017 •

edited

Loading

rimshaakhan Feb 20, 2017 •

edited

Loading

untitaker left a comment •

edited

Loading

untitaker Feb 20, 2017 •

edited

Loading

rimshaakhan commented Feb 21, 2017 •

edited

Loading

WhyNotHugo commented Feb 21, 2017 •

edited

Loading