Fix handling of datetime arguments to from_start_end by ederag · Pull Request #580 · projecthamster/hamster

ederag · 2020-03-08T20:40:03Z

Here is how I'd fix #576.

GeraldJansen · 2020-03-09T05:54:48Z

Hmm, tests/stuff_tests works fine, but hamster crashes:

Traceback (most recent call last):
  File "/usr/bin/hamster", line 149, in on_activate_window
    self._open_window(action.get_name(), data)
  File "/usr/bin/hamster", line 184, in _open_window
    self.overview_controller = Overview()
  File "/usr/lib/python3/dist-packages/hamster/overview.py", line 477, in __init__
    self.find_facts()
  File "/usr/lib/python3/dist-packages/hamster/overview.py", line 531, in find_facts
    self.facts = self.storage.get_facts(start, end, search_terms=search)
  File "/usr/lib/python3/dist-packages/hamster/client.py", line 156, in get_facts
    for fact in self.conn.GetFactsJSON(dbus_range, search_terms)]
  File "/usr/lib/python3/dist-packages/dbus/proxies.py", line 147, in __call__
    **keywords)
  File "/usr/lib/python3/dist-packages/dbus/connection.py", line 653, in call_blocking
    message, timeout)
dbus.exceptions.DBusException: org.freedesktop.DBus.Python.sqlite3.InterfaceError: Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/dbus/service.py", line 711, in _message_cb
    retval = candidate_method(self, *args, **keywords)
  File "/usr/libexec/hamster/hamster-service", line 335, in GetFactsJSON
    for fact in self.get_facts(range, search_terms=search_terms)]
  File "/usr/lib/python3/dist-packages/hamster/storage/storage.py", line 161, in get_facts
    return self.__get_facts(range, search_terms)
  File "/usr/lib/python3/dist-packages/hamster/storage/db.py", line 734, in __get_facts
    datetime_to))
  File "/usr/lib/python3/dist-packages/hamster/storage/db.py", line 906, in fetchall
    cur.execute(query, params)
sqlite3.InterfaceError: Error binding parameter 1 - probably unsupported type.

ederag · 2020-03-09T06:41:33Z

Confirmed, forgot to restart the server 🤦‍♂️ . Time to get out, I'm no longer into it...
I'll try to fix that later though.

matthijskooijman

Thanks for looking into this. I left some comments inline.

Also, I wonder if this is really a complete fix, since I suspect there might be two parts to this problem:

When a datetime instance is passed, it was previously treated as a date instance, throwing away time data.
When a date instance is passed, the returned range has an end datetime pointing at the first second of the next day.

Regarding the second, that suggests that the Range returned is intended to be used as a range from the start time up to excluding the end time, but it is actually interpreted to mean the entire day pointed to by that first second of the next day.

Looking even more closely, it seems that this is because from_start_end is called twice (once in client.get_facts and once in the server's GetFactsJSON). The second time it is called, problem 1 above causes the 1-second-into-the-next-day datetime to be converted into a date object for that next day, and the end result is a datetime 1-second-into-the-next-next-day, which is wrong.

So, I guess the fix is complete after all, since now it just passes datetime objects unchanged, so the server will not further modify it.

matthijskooijman · 2020-03-09T08:13:37Z

src/hamster/lib/datetime.py

-            range = Range(start, end)
+        elif isinstance(start, datetime):
+            # this one must come first,
+            # because isinstance(datetime(), date) is True


I was a bit confused by this comment, since it seems there is no isinstance(datetime(), date) below at all. Looking more closely, I see that pdt is actually the datetime import. Maybe this comment could be adapted too:

# because isinstance(datetime(), pdt.date) is True

That would have made things more clear for me.

date inherits from pdt.date.
I'll add a clarification though.

src/hamster/lib/datetime.py

mwilck · 2020-03-09T09:08:37Z

So we have this and #577, and both are closed ... ?

matthijskooijman · 2020-03-09T09:23:14Z

Maybe @ederag plans to submit a fixed PR later? If so, note that you can also do a (force) push to the branch for an existing PR to automatically update the PR (this is often better than starting a new PR, since any comment history will be kept).

ederag · 2020-03-09T09:50:38Z

Maybe @ederag plans to submit a fixed PR later?

Indeed (with force-push). Closed to lower the publicity.
Please just give me some time:

Confirmed, forgot to restart the server 🤦‍♂️ . Time to get out, I'm no longer into it...
I'll try to fix that later though.

It was a sunday evening quick fix, and it looks like I'm no longer into it.
I'll try to fix it this evening.

ederag · 2020-03-09T22:13:45Z

Got it (hopefully...). Just a few tests to do before pushing again.

matthijskooijman

Looks good to me, haven't tested it yet.

One question: If you pass a pdt.date, that is now handled, but I think not a date instance. It's probably not strictly needed (legacy uses pdt.date I suppose), but it would be easy to support date as well (just do isinstance(start, date) instead of isinstance(start, pdt.date). Or would it be better not to support this? Or would the Range constructor already accept date objects properly maybe?

ederag · 2020-03-13T21:01:16Z

Indeed, to avoid mistakes, I would restrict to hday for new code.
date is there mainly to be inherited by hday,
and as a temporary measure, until edit_activity.py has been updated to use hday.

matthijskooijman

Looks good, also good to choke on invalid types to prevent mistakes later on.

Looks good to merge like this, I think.

src/hamster/lib/datetime.py

matthijskooijman · 2020-03-14T08:52:28Z

Nit wrt the comment: AFAICS, datetime inherits only pdt.datetime, not date.

Good point, I added a suggestion for that.

can start be an instance of pdt.datetime() ? If yes, where is that case handled?

I think this is not a supported case. The old API was to pass pdt.date (or maybe date), the new API is hday, but it datetime might also be used when passing data through from_start_end twice, I think.

Writing this, I wonder if it would not be better to fix this by not passing datetime objects at all, just always stick to dates, but I guess you could also want to limit a range based on time too, so that would probably still need to work.

I previously said:

but it would be easy to support date as well (just do isinstance(start, date) instead of isinstance(start, pdt.date). Or would it be better not to support this? Or would the Range constructor already accept date objects properly maybe?

But that made no sense: The current isinstance(start, pdt.date) will already catch date instances, so it supports both date and pdt.date already.

Co-Authored-By: Matthijs Kooijman <matthijs@stdin.nl>

ederag · 2020-03-14T09:36:19Z

Range granularity is datetime only.

Otherwise, there would be ambiguities:
Suppose range = Range(hday1, hday2), what is the type of range.start ?

Passing hday to from_start_end() is only a convenience, to ease transition.
I'd recommend an explicit Range(hday1.start, hday2.end) for new code.

GeraldJansen · 2020-03-17T12:32:21Z

Free advice for new maintainers: This PR deals with a serious bug because it leads to incorrect invoices for anyone like me who relies on hamster export for invoicing. The bug should be fixed quickly and v3.0.2 should be released ASAP. Further code improvement can come later.

ederag · 2020-03-17T13:56:09Z

Indeed. And the code is fine now.
It is ready to ship, or are there any concerns ?

mwilck · 2020-03-17T16:03:13Z

Let's merge it, then. The 3.0.2 version bump should be discussed elsewhere.

This was referenced Mar 8, 2020

fix off-by-one bug for hamster export tsv #577

Closed

Off-by-one-day in hamster export tsv #576

Closed

matthijskooijman reviewed Mar 9, 2020

View reviewed changes

ederag closed this Mar 9, 2020

fix from_start_end: handle datetime first

93ab43f

ederag reopened this Mar 13, 2020

ederag force-pushed the master branch from fb1ae90 to 93ab43f Compare March 13, 2020 20:31

matthijskooijman approved these changes Mar 13, 2020

View reviewed changes

choke on invalid types in from_start_end

2af707c

matthijskooijman approved these changes Mar 13, 2020

View reviewed changes

matthijskooijman changed the title ~~Alternate fix for off-by-one bug~~ Fix handling of datetime arguments to from_start_end Mar 13, 2020

matthijskooijman reviewed Mar 14, 2020

View reviewed changes

src/hamster/lib/datetime.py Outdated Show resolved Hide resolved

fix inheritance order in comment

6754f66

Co-Authored-By: Matthijs Kooijman <matthijs@stdin.nl>

mwilck merged commit e6b90fc into projecthamster:master Mar 17, 2020

GeraldJansen mentioned this pull request Mar 17, 2020

Release v3.0.2? #588

Closed

Conversation

ederag commented Mar 8, 2020

Uh oh!

GeraldJansen commented Mar 9, 2020

Uh oh!

ederag commented Mar 9, 2020

Uh oh!

matthijskooijman left a comment

Choose a reason for hiding this comment

Uh oh!

matthijskooijman Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

ederag Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mwilck commented Mar 9, 2020

Uh oh!

matthijskooijman commented Mar 9, 2020

Uh oh!

ederag commented Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ederag commented Mar 9, 2020

Uh oh!

matthijskooijman left a comment

Choose a reason for hiding this comment

Uh oh!

ederag commented Mar 13, 2020

Uh oh!

matthijskooijman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthijskooijman commented Mar 14, 2020

Uh oh!

ederag commented Mar 14, 2020

Uh oh!

GeraldJansen commented Mar 17, 2020

Uh oh!

ederag commented Mar 17, 2020

Uh oh!

mwilck commented Mar 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ederag commented Mar 9, 2020 •

edited

Loading