Fix #117: add localtime to stop_date where clause

[chrisgurney]

Fix for #117, by adding "localtime" to the stop_date where clause.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[AlexanderWillner]

Thanks for the PR, lgtm, not tested though (see comments in #117)

[chrisgurney]

Setting the TZ environment variable using OS.environ between invocations of the things.py API appears to work on macOS, but not on a BSD system (or GitHub actions) per @mikez's test.

Next steps:

1. What is the way to change time zones mid script such that SQLite will notice the difference, and be able to query based on that?
   • Try this: Call time.tzset() after changing the TZ variable per: https://docs.python.org/3/library/time.html#time.tzset
2. After that's solved, how do I write a test for the .last API call (to test the createdDate query change in this PR) that gets the right amount of tasks from a past date? (stashed my first attempt)

[chrisgurney]

Forgot the other detail from my conversation with @mikez last week, which was that we think the call to .last only goes as far back as the current time X days ago, rather than midnight of that day.

So for example if I call the following...
last_tasks = things.last(f"{days_ago}d", status="completed")

...and it's currently 3:55pm, it only goes back days_ago days starting from 3:55pm on that day.

The last few lines of make_unixtime_range_filter show the query being constructed for .last:

    if suffix == "d":
        modifier = f"-{number} days"
    ...

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

    return f"AND {column_datetime} > {offset_datetime}"

So offset_datetime should be updated to look at midnight of today, before the modifier is applied.

In this context of this PR, this means I can't create a reliable test that will cover the change I made to make_unixtime_range_filter that accounts for different timezones.

[chrisgurney]

The decision to be made here, I think, is:

1. we commit and try the test above as-is; or
2. come to a decision on the behavior of .last and update it, and then complete this test so it covers both cases.
   • This change, by the way, might be as simple as adding a start of day modifier to the call to datetime -- I got the results I expected when I hacked this in, but this function might need further changes to account for the week and year modifiers.

Thoughts? @mikez @AlexanderWillner

[mikez]

👍

I vote for splitting this into two PRs.

As for the behavior of "last", maybe @lmgibson can help us here as he was the architect here. In essence, the question is whether last='2d' should mean the last 48h from the time right now or the last two dates.

[chrisgurney]

@mikez Marked as Ready for Review per your approval.

@AlexanderWillner I believe this is over to you now (unless you disagree).

• Note that there's a possibility the tests won't pass when run by GitHub, as it was not previously recognizing the TZ environment variable change mid-execution. Hoping this is now resolved by calling time.tzset().

[mikez]

@chrisgurney I ran the tests. There seem to be some doctest failures still. You can run these with make testdoc.

[chrisgurney]

Update: I also needed to install pytest-cov
pip install pytest-cov

...otherwise I was getting errors running make testdoc:

% make testdoc       
THINGSDB=tests/main.sqlite pytest --cov=things -W ignore::UserWarning --cov-report=xml --cov-context=test --doctest-modules
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov=things --cov-report=xml --cov-context=test

[chrisgurney]

@mikez Back to you.

[mikez]

@AlexanderWillner If you'd enjoy to do so, can you add a check for pytest-cov in the makefile; see #118 (comment)

[mikez]

@chrisgurney make lint and make auto-style may be helpful for the current failures.

[chrisgurney]

@AlexanderWillner If you'd enjoy to do so, can you add a check for pytest-cov in the makefile; see #118 (comment)

Here's what I was thinking:

@type pytest >/dev/null 2>&1 || (echo "Run '$(PIP) install pytest; $(PIP) install pytest-cov' first." >&2 ; exit 1)

[mikez]

@AlexanderWillner If you'd enjoy to do so, can you add a check for pytest-cov in the makefile; see #118 (comment)

Here's what I was thinking:

@type pytest >/dev/null 2>&1 || (echo "Run '$(PIP) install pytest; $(PIP) install pytest-cov' first." >&2 ; exit 1)

You can do multiple arguments to pip: pip install pytest pytest-cov.

[chrisgurney]

@mikez Linting issues fixed. Thanks for the tips.

[mikez]

@chrisgurney 🥳🙌

[chrisgurney]

Everything passed!

For the future I think it would also be helpful to add a CONTRIBUTING file to the project that:

1. suggests the make commands to run prior to submitting a PR, which for me was test, testdoc, and lint;
2. talks to how to copy over the test Things database (after backing up your own) via make db-to-things and make db-from-things, as well as:
   • a disclaimer about making changes to it (schema knowledge perhaps being a pre-requisite here);
   • how to restore your original/personal Things database, as todos may have changed on other devices during development
3. @mikez helped me with was creating a venv, which was new to me as a relatively new Python developer -- might be helpful to point to the docs for that;

[mikez]

@chrisgurney Good idea! Do you want to craft such a simple file?
You could model it on other CONTRIBUTING.md files that you enjoy.

[chrisgurney]

@chrisgurney Good idea! Do you want to craft such a simple file? You could model it on other CONTRIBUTING.md files that you enjoy.

Thought you might suggest that. 😅 Perhaps when I come back to the second test, once (if) .last is updated.

[mikez]

@chrisgurney I think you're almost there. If you give #118 (comment) as an input to some GPT, I'm sure it can make a Contributing.md for you. :)