Patch to show for #79: show "yellow" To-Dos in Today and remove them from Upcoming #80

AlexanderWillner · 2021-05-05T18:52:26Z

No description provided.

…sks)

codecov-commenter · 2021-05-05T18:53:10Z

Codecov Report

Merging #80 (747cae0) into main (4141f5c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #80   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          341       352   +11     
=========================================
+ Hits           341       352   +11

Impacted Files	Coverage Δ
things/api.py	`100.00% <100.00%> (ø)`
things/database.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4141f5c...747cae0. Read the comment docs.

AlexanderWillner · 2021-05-05T18:56:22Z

@mikez will test tomorrow when having yellow tasks - feel free to already comment on the changes.

mikez · 2021-05-06T15:01:57Z

I took a look!

I'm not a big fan of leaking SQL into the API interface, since it adds complexity. Alternative idea: Since the number of today tasks and also those " > now" are practically a small number, you could do this in pure Python. For example, using list comprehensions or a for loop and the datetime.now().isoformat() output.
To clarify what I wrote here: When you click on "Today" in the Things app, then yellow notifications will be generated for various cases. You are only handling one of these here. I suggest you think about and list out all of the unconfirmed_today_tasks cases as a comment in things.today. This will give us an overview over what we've already implemented and what we haven't. Otherwise, users won't know what to expect.

mikez · 2021-05-07T10:11:47Z

Another idea for (1.): add the string options "past" and "future" to start_date. It compares start_date to the date and time of now.

Also, a technical remark: I've noticed that start_date isn't always a date. For example, if you set an alert, then it turns into a datetime.

mikez · 2021-05-08T22:20:58Z

Some suggestions:

Why title-case "Future" and "Past" as supposed to "future" and "past"?
To not increase complexity, I'd keep database.make_filter as it was before and instead extend database.make_date_filter. You're essentially doing make_date_filter with an offset of "0" here. For boolean offsets, you can call database.make_filter from within make_date_filter.
Add start_date to the "# Validation" section.
The parameter deadline is very similar, you might consider giving it the past/future options too.

AlexanderWillner · 2021-05-09T21:09:12Z

Why title-case "Future" and "Past" as supposed to "future" and "past"?

No special reason, changed it now. Also created #81 because of this, as currently we have for example Inbox (uppercase) but incomplete (lowercase).

To not increase complexity, I'd keep database.make_filter as it was before and instead extend database.make_date_filter. You're essentially doing make_date_filter with an offset of "0" here. For boolean offsets, you can call database.make_filter from within make_date_filter.

I gave it a shot.

Add start_date to the "# Validation" section.

Done, however, see #81 - I'm thinking more about exploiting types (enums, classes, ...) or type hints here sooner or later.

The parameter deadline is very similar, you might consider giving it the past/future options too.

Done.

mikez · 2021-05-10T15:52:50Z

Why title-case "Future" and "Past" as supposed to "future" and "past"?

No special reason, changed it now. Also created #81 because of this, as currently we have for example Inbox (uppercase) but incomplete (lowercase).

The convention I used here was to always do lower case except for where the Things App has a different convention. "Inbox", "Anytime", etc. are special nouns involving GTD collection names; they're title-cased in the app. It is fine to use start="inbox" though, since we run the line start = start and start.title() in database.get_tasks.

mikez · 2021-05-10T15:54:06Z

I will take a look at your changes and comment as soon as possible.

…ion date) tasks with a due date, calculated some test expectations, reordered some expectations

mikez

Thanks for your patience, @AlexanderWillner. Commented. I hope it's helpful.

things/api.py

things/database.py

mikez · 2021-05-25T20:15:57Z

things/database.py

    return {
        None: "",
        False: f"AND {column} IS NULL",
        True: f"AND {column} IS NOT NULL",
    }.get(value, default)


-def make_date_filter(date_column, offset):
+def make_date_filter(date_column: str, offset) -> str:


I think this function is getting too complex. I find it hard to read.
Maybe split it out into two functions?

make_date_range_filter ("3d", "2w", "5y")

make_date_filter (True, False, "future", "past")

Personal gut feeling: keep it. These functions would be very similar in what they want to achieve and the current version is around 20 lines of code. But also fine splitting it.

I vote for splitting. It seems hard to read, especially for somebody who's never seen this code before.

A date range filter (specified by an offset) and a date filter (True, False, "past", "future) seems more readable for me.

One can always merge again if it seems too redundant. I much prefer many smaller functions.

Created #82 for this

Why break it out into a separate PR?

The proposal was to rename make_date_filter to make_date_range_filter, but leave it as-is. Then, to add a new function called make_date_filter which operates on True, False, None, "future", "past".

Mh:

In case of None: return ""

In case of True/False: return make_filter

In case of Future/Past: set offset to 0 and specify operator

The new method would duplicate the actual code

column_datetime = f"datetime({date_column}, 'unixepoch', 'localtime')" offset_datetime = f"datetime('now', 'localtime', '{modifier}')" # type: ignore

Wouldn't it?

For the case of "future" and "past", my thinking was to call make_date_range_filter from within make_date_filter. Or.. what do you think?

@AlexanderWillner I think we're almost there with this review. I look forward when we can merge this in. :)

things/api.py

mikez · 2021-05-27T21:19:38Z

things/api.py

+    unconfirmed_overdue_tasks = tasks(
+        start_date=False,
+        deadline="past",
+        start="Anytime",


Wouldn't Inbox and Someday be possible too? Maybe the start is irrelevant here.

ping @AlexanderWillner ; see earlier comment further above which wasn't resolved yet.

things/api.py

things/database.py

mikez · 2021-05-27T21:28:09Z

things/database.py

    return {
        None: "",
        False: f"AND {column} IS NULL",
        True: f"AND {column} IS NOT NULL",
    }.get(value, default)


-def make_date_filter(date_column, offset):
+def make_date_filter(date_column: str, offset) -> str:


I vote for splitting. It seems hard to read, especially for somebody who's never seen this code before.

A date range filter (specified by an offset) and a date filter (True, False, "past", "future) seems more readable for me.

mikez · 2021-05-27T21:29:26Z

things/database.py

    return {
        None: "",
        False: f"AND {column} IS NULL",
        True: f"AND {column} IS NOT NULL",
    }.get(value, default)


-def make_date_filter(date_column, offset):
+def make_date_filter(date_column: str, offset) -> str:


One can always merge again if it seems too redundant. I much prefer many smaller functions.

things/api.py

AlexanderWillner · 2021-05-30T12:39:39Z

@mikez long discussions here ;) remind me: is there something open?

mikez · 2021-05-31T22:17:56Z

things/api.py

        - `deadline == None` (default), then include all tasks.

+    deadline_suppressed : bool or None, optional
+        - `deadline_suppressed == True`, only include tasks with a overdue deadline


"with an overdue deadline"

might need more explanation: "deadline suppressed" is a technical term used in the database. It means overdue tasks which have been moved from Today to Inbox, Anytime, or Someday.

mikez · 2021-05-31T22:19:54Z

things/api.py

+        - `deadline_suppressed == True`, only include tasks with a overdue deadline
+          that have been moved from the today view (Inbox, Anytime, Someday).
+        - `deadline_suppressed == False`, only include tasks with an overdue deadline
+          that have not been moved from the today view to another view (Inbox, Anytime, Someday).


Are only tasks with overdue deadlines included or all tasks no matter if their deadline is overdue or not?

mikez · 2021-05-31T22:21:51Z

things/api.py

+    unconfirmed_overdue_tasks = tasks(
+        start_date=False,
+        deadline="past",
+        start="Anytime",


ping @AlexanderWillner ; see earlier comment further above which wasn't resolved yet.

mikez · 2021-05-31T22:26:12Z

things/database.py

    return {
        None: "",
        False: f"AND {column} IS NULL",
        True: f"AND {column} IS NOT NULL",
    }.get(value, default)


-def make_date_filter(date_column, offset):
+def make_date_filter(date_column: str, offset) -> str:


Why break it out into a separate PR?

The proposal was to rename make_date_filter to make_date_range_filter, but leave it as-is. Then, to add a new function called make_date_filter which operates on True, False, None, "future", "past".

things/api.py

AlexanderWillner · 2021-06-05T07:55:14Z

@mikez let us finish this pull request ;)

mikez · 2021-06-05T16:20:50Z

@AlexanderWillner Thanks for your patience. I hope to get back to this PR ASAP.

mikez · 2021-06-06T15:38:46Z

@AlexanderWillner Well done. I'll satisfy my need for order and readability in an upcoming pull request.

AlexanderWillner · 2021-06-06T19:03:45Z

Thanks. Does it? You changed a few things (for the better) in #83 - good work ;)
PS: misread your initial comment.

initial (untested) starting point for thingsapi#79 (showing yellow ta…

b0ef58d

…sks)

tested yellow tasks

53964fe

AlexanderWillner changed the title ~~initial (untested) starting point for #79 (showing yellow tasks)~~ Patch to show for #79: show "yellow" To-Dos in Today and remove them from Upcoming May 6, 2021

don't expose SQL into the API

954dafd

updates after review

95018cb

AlexanderWillner added 2 commits May 20, 2021 10:32

cleanup comments for thingsapi#79

20ffd77

tested for yellow (no suppression date) and non-yellow (with suppress…

54f3b64

…ion date) tasks with a due date, calculated some test expectations, reordered some expectations

mikez reviewed May 25, 2021

View reviewed changes

changes after review

1897048

mikez requested changes May 27, 2021

View reviewed changes

AlexanderWillner added 4 commits May 28, 2021 08:00

reflect sort order in today view

900a94c

test for 900a94c (correct order in today view)

c8b4fdf

suppressed -> deadline_suppressed

de9b9c1

updated today description (yellow tasks)

4800480

AlexanderWillner mentioned this pull request May 30, 2021

Split make_date_filter #82

Closed

AlexanderWillner added 2 commits May 30, 2021 14:36

removed comma

09a0751

removed old utility functions

f1cd8c7

mikez requested changes May 31, 2021

View reviewed changes

AlexanderWillner added 3 commits June 2, 2021 21:58

removed start date for unconfirmed_overdue_tasks

354766f

typo

0b5688e

explain suppressed deadline

7ca7a82

AlexanderWillner added 2 commits June 2, 2021 22:16

clarify suppressed deadline flag

62c66b8

refactor to add make_date_range_filter

747cae0

mikez merged commit ab73287 into thingsapi:main Jun 6, 2021

mikez mentioned this pull request Jun 6, 2021

Update documentation and SQL date filters in database. #83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch to show for #79: show "yellow" To-Dos in Today and remove them from Upcoming #80

Patch to show for #79: show "yellow" To-Dos in Today and remove them from Upcoming #80

AlexanderWillner commented May 5, 2021

codecov-commenter commented May 5, 2021 •

edited

Loading

AlexanderWillner commented May 5, 2021

mikez commented May 6, 2021

mikez commented May 7, 2021 •

edited

Loading

mikez commented May 8, 2021

AlexanderWillner commented May 9, 2021

mikez commented May 10, 2021

mikez commented May 10, 2021

mikez left a comment

mikez May 25, 2021

AlexanderWillner May 27, 2021

mikez May 27, 2021

mikez May 27, 2021

AlexanderWillner May 30, 2021

mikez May 31, 2021

AlexanderWillner Jun 2, 2021

mikez Jun 2, 2021

mikez May 27, 2021

mikez May 31, 2021 •

edited

Loading

mikez May 27, 2021

mikez May 27, 2021

AlexanderWillner commented May 30, 2021

mikez May 31, 2021

mikez May 31, 2021

mikez May 31, 2021 •

edited

Loading

mikez May 31, 2021

AlexanderWillner commented Jun 5, 2021

mikez commented Jun 5, 2021

mikez commented Jun 6, 2021

AlexanderWillner commented Jun 6, 2021 •

edited

Loading

Patch to show for #79: show "yellow" To-Dos in Today and remove them from Upcoming #80

Patch to show for #79: show "yellow" To-Dos in Today and remove them from Upcoming #80

Conversation

AlexanderWillner commented May 5, 2021

codecov-commenter commented May 5, 2021 • edited Loading

Codecov Report

AlexanderWillner commented May 5, 2021

mikez commented May 6, 2021

mikez commented May 7, 2021 • edited Loading

mikez commented May 8, 2021

AlexanderWillner commented May 9, 2021

mikez commented May 10, 2021

mikez commented May 10, 2021

mikez 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

mikez May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderWillner commented May 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikez May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderWillner commented Jun 5, 2021

mikez commented Jun 5, 2021

mikez commented Jun 6, 2021

AlexanderWillner commented Jun 6, 2021 • edited Loading

codecov-commenter commented May 5, 2021 •

edited

Loading

mikez commented May 7, 2021 •

edited

Loading

mikez May 31, 2021 •

edited

Loading

mikez May 31, 2021 •

edited

Loading

AlexanderWillner commented Jun 6, 2021 •

edited

Loading