things.py

things.py copied to clipboard

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

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

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

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)

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.

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

👍

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.

@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().

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

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

@mikez Back to you.

@AlexanderWillner If you'd enjoy to do so, can you add a check for pytest-cov in the makefile; see https://github.com/thingsapi/things.py/pull/118#issuecomment-2189113454

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

@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)

@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.

@mikez Linting issues fixed. Thanks for the tips.

@chrisgurney 🥳🙌

Everything passed!

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

1. @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;
2. 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); open the Things app to validate your changes are as expected;
   • how to restore your original/personal Things database, as todos may have changed on other devices during development -- note that you may have to log back into Things Cloud
3. the make commands to run prior to submitting a PR, which for me was:
   • test runs the tests in test_things.py -- make sure you've copied the test database to your Things app folder (after backing it up first)
   • testdoc validates that the examples provided in the documentation produce the results expected by changes to the API
   • lint ensures code changes comply with linting rules; make any changes reported after running this

@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 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.

@chrisgurney I think you're almost there. If you give https://github.com/thingsapi/things.py/pull/118#issuecomment-2189233210 as an input to some GPT, I'm sure it can make a Contributing.md for you. :)

@mikez I've got a draft of CONTRIBUTING.md almost ready. I'll submit a PR soon.