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

Things.py Refactor #2

Merged
merged 3 commits into from
Jan 10, 2024
Merged

Things.py Refactor #2

merged 3 commits into from
Jan 10, 2024

Conversation

mikez
Copy link
Contributor

@mikez mikez commented Dec 31, 2023

If you have any questions, feel free to post here and I will respond.

@chrisgurney
Copy link
Owner

@mikez Wow, this is very generous of you! Sorry I'm only seeing this now. I'll take a look when I'm able, and see if it makes sense for me. I truly appreciate it.

@chrisgurney chrisgurney merged commit 9745cba into chrisgurney:main Jan 10, 2024
@chrisgurney
Copy link
Owner

chrisgurney commented Jan 10, 2024

@mikez I'm starting to test this merge, and I'm not getting results. Example of the "Today" query, which boils down to just the following, is returning zero results:

import things

kwargs = dict()

kwargs['start_date'] = True
kwargs['status'] = 'incomplete'
kwargs['start'] = 'Inbox'

tasks = things.tasks(**kwargs)

print(tasks)

@mikez
Copy link
Contributor Author

mikez commented Jan 10, 2024

@chrisgurney Glad to see you playing around with it! Let's debug together.

First:

>>> import things
>>> things.today()

is this working? You can see the source of it here.

It first does a regular fetch for tasks with start="Anytime" (today's items tend to be there), see also terminology here. Then it does two "predictive" fetches for tasks that would end up there once Things.app opened for the day.

@chrisgurney
Copy link
Owner

chrisgurney commented Jan 10, 2024

@mikez Excellent, that pointer to the source helped a lot with selecting the proper kwargs, thanks! Got the query working again, so I can make progress with testing at least!

I do see that "Evening" tasks are at the top of the list, whereas if you add an ORDER BY startBucket to the SQL query (see my original "v1" script for reference), they'll be pushed to the bottom properly, as they appear in the Things "Today" view.

I attempted to do a tasks.sort() on startBucket and got an error.

Have you looked into understanding and/or adding support for the startBucket column at all? I don't see any mention of it in the things.py source, nor in GitHub Issues.

If I can figure out the source, I'm tempted to take a stab at a PR, but will likely need assistance, I'm sure. :-)

@mikez
Copy link
Contributor Author

mikez commented Jan 10, 2024

@chrisgurney I was not aware of startBucket before; that sounds like a great addition!

To clarify (asking out loud): what does startBucket track? Is it if a task is set to "evening" or not? So it has all the same properties as a Today task just with startBucket=1? Are there other possible startBucket values?

I see three options so far:

  1. We could expose the startBucket as a key in the result, maybe "evening: True"? Then you could do tasks.sort in Python.
  2. We could add the startBucket as an index, akin to todayIndex. However, in looking at the database, it seems startBucket is not set as an index.
  3. We add the concept of "order_by" as a first-class citizen to things.py, so you can order by whatever field(s) you want and don't have to do it through Python. (So far, the Python sort-method has worked great for me personally.)

What do you think?

@chrisgurney
Copy link
Owner

chrisgurney commented Jan 10, 2024

@mikez In my limited testing when looking at the Today list, when moving a task between Evening and not-the-Evening (and vice-versa) the startBucket value changes between 1 (Evening) and 0 (not Evening).

I also tried moving tasks around in Anytime and Someday (i.e., other start possibilities), and didn't see a change in that value, which I think makes sense as there aren't any other special "buckets" tasks can go in, in those views. Thus, beyond the Today view, I'm not sure how else it's used.

Given the above, I do like solution 1, where you suggested "evening: True". However, if Cultured Code is thinking of adding other buckets in future releases that are under those views, a boolean might not cut it, and an enum might be a better fit.

If I put my DB design hat on, it makes me wonder why they chose the generic "bucket", versus just using an "evening" flag. But that may be because "Evening" wouldn't apply to most tasks, and wanted to leave it open to other uses in the future. This leads me back to thinking that an enum might be a safer bet.

Thoughts? 😄

@chrisgurney
Copy link
Owner

FYI you should know I'm relatively new to Python (not sure if that was obvious looking at my code), so your knowledge of the appropriate data structures to solve this will most definitely exceed mine. 😅

@chrisgurney chrisgurney changed the title To help you get started... :) Things.py Refactor Jan 10, 2024
@mikez
Copy link
Contributor Author

mikez commented Jan 10, 2024

@chrisgurney Thank you for the analysis!

I took a look at the guiding design principle I chose back in the day:

The terms of the API aim to match those used in the Things app and
Things URL Scheme. In some specific cases we instead choose to use terms
that occur in the Things SQL database to closer reflect its underlying
data structures.

As much as I would have loved {"evening": True}, I concur with your analysis and think {"start_bucket": "Evening"} is the way to go; my UX sensibilities are shuddered looking at that though... Maybe they thought of "start_bucket" as a sub-hierarchy of the "start" field? What's also confusing, is that the Things URL Scheme uses a "when" field (see here), however that doesn't occur at all in the database.

Would you enjoy to give a PR a shot? Note there's a database in the tests/ directory where you might add a to-do and/or project with start_bucket=1 to? Happy to guide, we can do that in the PR itself.

FYI you should know I'm relatively new to Python [...]

Welcome to Python! 🐍 What programming languages are you most familiar with?

@chrisgurney
Copy link
Owner

@mikez Thanks for the feedback! I'll put some thought into the start_bucket PR when I'm able -- I'm looking forward to contributing to my first OSS project!

In other news, I've got what I think are all the use cases for things2md working with Things.py, with just a bit more regression testing to do... and adding support for start_bucket of course.

🧑‍💻 I went to school back when Java was just arriving on the scene, so got into that and then J2EE. At the time I used PHP for personal projects. In more recent years, I've been getting into React (for working with design systems as a UX designer), and now Python for personal projects (mostly for scripting and static site generation).

Thanks again for the help with future-proofing my lowly personal utility script. 😄

@mikez
Copy link
Contributor Author

mikez commented Jan 12, 2024

@chrisgurney As soon as you have some code, feel free to submit a PR request, and we can take it from there.

@mikez
Copy link
Contributor Author

mikez commented Jan 12, 2024

Looking through your code, note that in the API you can do

things.tasks(last='3d')
things.tasks(last='2w')
things.tasks(last='1y')

Although, I normally call things.todos(last=…) or things.projects(last=....) and not the more generic things.tasks.

@chrisgurney
Copy link
Owner

Looking through your code, note that [...] you can do

things.tasks(last='3d')
things.tasks(last='2w')
things.tasks(last='1y')

Oh that's much cleaner than all those date calculations. I'll take a look to see if that works for me. Thanks!

@chrisgurney
Copy link
Owner

chrisgurney commented Feb 25, 2024

Hey @mikez I could use your help:

I'm adding an argument to things2md to get all tasks completed on a given ISO date, and it seems to be returning tasks from the day before. Is this a timezone issue? (I think some of my other options are off by a day as well, but let's start here.)

Example: I'm looking for all tasks completed on ARG_DATE = 2024-02-25

Code:

kwargs['status'] = None
kwargs['stop_date'] = f'{ARG_DATE}'
tasks = things.tasks(**kwargs)

Here's the stopDate for one of the returned tasks: 1708826542.13277

GMT: Sunday, February 25, 2024 2:02:22.132 AM
Your time zone: Saturday, February 24, 2024 9:02:22.132 PM GMT-05:00

IF things.py doesn't support time zone adjustments, how would you do a query like this?

Thanks!

@chrisgurney
Copy link
Owner

chrisgurney commented Feb 25, 2024

@mikez I've added support for the --date argument to things2md as it stands at the moment, if you'd like to see how it's behaving with your own data.

I'll update here if I figure out any workarounds. Thinking of: getting tasks completed on the date provided and the next day's, then filtering the returned task times against the local time.

@mikez
Copy link
Contributor Author

mikez commented Feb 25, 2024

@chrisgurney I hope to take a closer look at your question Monday or Tuesday.

@chrisgurney
Copy link
Owner

@chrisgurney I hope to take a closer look at your question Monday or Tuesday.

Appreciate it, thanks!

@chrisgurney
Copy link
Owner

chrisgurney commented Feb 26, 2024

@mikez Update: I think I have a fix in place (given my workaround noted previously) but it's anything but elegant.

@mikez
Copy link
Contributor Author

mikez commented Feb 27, 2024

@chrisgurney I took a look. I think you spotted a bug, or rather, something that wasn't thought of yet amidst the recent date updates.

Currently:

  • in responses, stop_date, modified, and created are in "localtime";
  • in queries, stop_date expects GMT (aka UTC) dates.

The database itself stores these dates as UTC.

Suggested Changes

Make both responses and queries be in localtime. What do you think?

To do so, tweak these functions in database.py of things.py:

Want to give it a shot? Hopefully, these should be easy edits and not involve much more than adding "localtime" to the SQL-query, i.e., date(stopDate, 'unixepoch', 'localtime') instead of date(stopDate, 'unixepoch').

Let me know if you have any questions; happy to guide.

@chrisgurney
Copy link
Owner

chrisgurney commented Feb 29, 2024

Hey @mikez thanks for following up!

Just yesterday I went through a similar issue with PyGithub and had to implement the same workaround.

You might want to take a look through this thread to see if this changes your thinking at all.

My main concern would be: Is this a breaking change for implementers of things.py who will now get a different result set? Perhaps the tasks function should accept a datetime object wherever a date is accepted, as opposed to an ambiguous string? IMaybe it could be introduced as an option, while the date string version of the argument is marked as deprecated, somehow. Is that possible? What do you think?

I've almost implemented my own workaround in things2md, now that I have a better understanding of how 🐍 datetime and timezones work. But I do agree that it such a change make the things.py API more intuitive, and is worth doing.

@mikez
Copy link
Contributor Author

mikez commented Feb 29, 2024

Thank you for the the link to the thread and proposals. I pondered a bit. :)

Two UX questions:

  • What's an example, where you'd not want to use your local time for the stop_date parameter? (It seems to me, when you use the Things app, it sets the stop_date using the local timezone. That the DB stores it in UTC, I'd say is an implementation detail. If we do find such salient examples, then we could use ISO 8601 datetime strings like "2024-02-29T14:55:21−07:00" or Python datetime; then you have fine granular control.)
  • Can you think of an example, where having Python's datetime is preferable over ISO 8601 date strings? (I find strings way easier to type as a user, than having to import libraries and set the correct timezone.)

@mikez
Copy link
Contributor Author

mikez commented Mar 1, 2024

Some Python tricks (looking at your code):

>>> import datetime
>>> first_datetime = datetime.datetime.today()
>>> first_datetime.date().isoformat()
'2024-03-01'
>>> stop_date = '2024-03-10'
>>> datetime.date.fromisoformat(stop_date)
datetime.date(2024, 3, 1)

(Similar methods exist for dates with times and timezones.)

@chrisgurney
Copy link
Owner

chrisgurney commented Mar 4, 2024

Hey @mikez, thanks for checking in.

I'm in the middle of re-writing things2md to use a templating system. Once that's done (hopefully in the next day or two), I'll be able to get back to sorting out the date issue; I might take you up on your offer for a call.

In terms of use cases, I can only speak for what my own users of things2md are using it for: exporting tasks completed on a given date (local time) for keeping a record. It seems like the majority of the implementations of your library are using it for personal reporting or keeping a record as well, but I'd be curious what other implementers of things.py think.

I suspect that the users of those projects are assuming their local time is applied (and haven't spotted the issue), or those projects have implemented their own workarounds.

More thoughts re: Strings:

  1. Agree that entering a date string is preferable for simple usage / quick implementation.
  2. However, it assumes the user "speaks" in ISO dates, and then requires conversion from dates that might be expressed in other ways.
  3. Does things.py support times? It seems like tasks(start_date) doesn't, according to the docs? I'm curious if supporting times might be useful (e..g, "get only tasks completed this morning").

@mikez
Copy link
Contributor Author

mikez commented Mar 4, 2024

As for times:

@chrisgurney
Copy link
Owner

@mikez Quick update: Hoping to look at this soon!

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.

2 participants