nexedi / zodbtools

@kirr @Tyagov @klaus @kazuhiko this is following up !7 (comment 72207)

Two things needs to be double checked, I think:

is the conversion "date -> zodb transaction id" correct ?
is this optimisation from d56fcf2c acceptable ? It can be a problem if user provide a date in a format that is exactly 16 characters long and only contain characters in the range [0-9][A-F][a-f] so I don't think it will happen. With this optimisation test suite goes from 8.69 to 0.66 seconds, dateparser is really slow on "invalid" inputs.

To manually verify, I used

from ZODB import FileStorage, DB
import transaction
import datetime

storage = FileStorage.FileStorage('demo.fs')
db = DB(storage)
connection = db.open()
root = connection.root()
now = str(datetime.datetime.now())
root['now'] = now
print ("adding transaction", now)

transaction.commit()

ran python demo.py several times and check with zodb analyze demo.fs "..1 minutes ago" and similar relative times and it looks OK.

@jerome, thanks the patches. So far I've only very briefly checked them (I will try to have a closer look in a day or two) and it looks generally ok. Some review points:

could we also test relative date parsing? (we can try to patch time.time to return something expected, e.g. 1.jan.2000, and then relative timestamps should be known)
could we please try to make sure we support the same timestamp syntax as in git? It is very unnatural to see spaces in a tid range and without proper quoting it breaks. If I understand correctly git supports using . or _ inside, e.g.:

kirr@link:~/src/wendelin/erp5$ git log -1 --until=1.jan.2010
commit 374799a647a6c1a653399086d396c769dc2ef81e
Author: Fabien Morin <fabien@nexedi.com>
Date:   Thu Dec 31 16:41:03 2009 +0000

    move listbox_description to hidden group

    git-svn-id: https://svn.erp5.org/repos/public/erp5/trunk@31555 20353a03-c40f-0410-a6d1-a30d3c3de9de

kirr@link:~/src/wendelin/erp5$ git log -1 --until=1_jan_2010
commit 374799a647a6c1a653399086d396c769dc2ef81e
Author: Fabien Morin <fabien@nexedi.com>
Date:   Thu Dec 31 16:41:03 2009 +0000

    move listbox_description to hidden group

    git-svn-id: https://svn.erp5.org/repos/public/erp5/trunk@31555 20353a03-c40f-0410-a6d1-a30d3c3de9de

we also have go version of zodbtools (here: https://lab.nexedi.com/kirr/neo/tree/master/go/zodb/zodbtools, https://lab.nexedi.com/kirr/neo/tree/master/go/zodb/cmd/zodb) and it would be good if we define a date syntax so that we could keep two versions interoperable. The defined set could be smaller than what is completely suppored by dateparser, but still it would be good if we could have the core set properly defined (by examples / tests is enough) to be there. For the reference here is ZODB/go code that is converting a tid to time: https://lab.nexedi.com/kirr/neo/blob/master/go/zodb/time.go, https://lab.nexedi.com/kirr/neo/blob/master/go/zodb/time_test.go
I suggest to consider to switch testing from ranges to testing of tids. The range parser is just a) split by .. and then parse each tid individually. We could have a good coverage of all veriants for tids, and only a few variants of tidrange (assuming that tid parsing is working). Testing just tids should be easier and with higher signal / noise.
I think it is a good approach to precheck whether a tid is raw hex. imho we should: 1) check if a tid is 16 character in length; 2) if yes - directly try to parse that as hex number.
I suggest to consider using time in internal nameing instead of date. To me date suggests it is a day, but tids could be corresponded to any timestamp, e.g. 2nd day, 10 hours, 3 minutes, xx seconds. For the latter time or timestamp, imho, corresponds more closely. I thus suggest to use parse_tid(tid_string, raw_only=False) for such parsing and internally it would try to decode hex and then tail to dateparser. And if raw_only is True, it would try to decode only hex. How do you think - does it make sense?

Once again the changes look generally good. Let me plase have a closer look in some time, and then I hope we should merge without much back and forth.

Thanks again, Kirill

@kirr thanks for feedback. All this makes sense, I'll rework these patches according to your suggestions in the next few days.

For the records,

zodb analyze ./demo.fs  1_jan_2010..
zodb analyze ./demo.fs  1.jan.2020..

are both currently supported formats.

I could not find reference of git supported formats, only this test https://github.com/git/git/blob/v2.20.1/t/t0006-date.sh

Since there's also another go implementation (I did not know that), then we should not just "support anything that dateparser supports" but officially support a common syntax. I see golang has https://github.com/araddon/dateparse or https://github.com/olebedev/when that also seem to support several formats.

@jerome, thanks for feedback, for links to go human-timestamp parsing libraries, and for being open to rework your patches. It is good dateparser supports . and _. To ease interoperability testing between py and go tid parsing, I thus sugest for parse_tid input vector to reside in e.g. tid_test.txt in

hex-tid  human-readble-tid-til-EOL
----//----
...

format. Then on go side the tests would just verify against the same input vector and it would be relatively easy to keep both parsers in sync.

Looking forward for the updated patches from you.

For the reference (I thought I was writing about it already) we not only have zodb/go tools, but also FileStorage/go specific tools, that are a superset in some aspects compared to python version:

https://lab.nexedi.com/kirr/neo/tree/master/go/zodb/storage/fs1/fs1tools
https://lab.nexedi.com/kirr/neo/tree/master/go/zodb/storage/fs1/cmd/fs1

For the reference 2: for my wcfs work for testing I taught fs1/go driver to also support notifying clients about the changes made to the database externally (in zodb/py this is supported by ZEO and NEO but not FileStorage). Here are relevant bits: https://lab.nexedi.com/kirr/neo/blob/22a4acd4/go/zodb/storage/fs1/filestorage.go#L474

marked as a Work In Progress

There have been some progress on this, first focusing on what should be the supported time formats. I pushed some experiments in another branch to create that file with supported inputs. I started from that git log test and adjusted a bit to remove some unsupported formats.

Supported formats is https://lab.nexedi.com/jerome/zodbtools/blob/feat/time-range-spec/zodbtools/test/testdata/tidrange-formats.txt and the prototype code to read them (in python and go) are in https://lab.nexedi.com/jerome/zodbtools/tree/feat/time-range-spec/prototype

This is a first step where it looks OK when local time is UTC, but I need to think more about how this should honour local timezone. I just wanted to make a quick progress report for now.

@jerome, thanks for the update. I think for that file it would be good to leave the first field as RFC3339 - then for human readable dates we verify they are converted to valid UTC timestamp, and then UTC timestamps are in direct correspondence with TIDs. Leaving correct values as time should make that test vector maintainance easier.

Overal it goes into good direction. Thanks again for the update,
Kirill

added 1 commit

c1004a3a - tidrange: supports giving range as time

Compare with previous version

unmarked as a Work In Progress

@kirr I just pushed updated patches which - I think - addressed everything we talked about. Two points:

because dateparser uses datetime, it cannot be patched easily, as you can see below. To make this easier,I used https://github.com/spulec/freezegun (as a test dependency)

import datetime
import mock
now = datetime.datetime.now()
with mock.patch("datetime.datetime.now", return_value=now):
    print (datetime.datetime.now())
# TypeError: can't set attributes of built-in/extension type 'datetime.datetime'

In the process, I updated my notes on supported formats https://lab.nexedi.com/jerome/zodbtools/tree/feat/time-range-spec/prototype and have a small concern that the dateparser does not detect invalid inputs and this might lead to unexpected behaviours for users ( see this paragraph of the notes )

@jerome, thanks for update.

I see about freezegun - indeed datetime is C-level module that uses gettimeofday and patching just time.time does not affect it. For me it is ok to use freezegun for testing.

About dateparser not detecting invalid input - I've tried dateparser master and for some of your example invalid inputs it does detect the badness:

In [11]: dateparser.parse('2009-06-01T10:00:00:+09:00')

In [12]: dateparser.parse('2009-06-34T22:00:00Z')

(with None returned indicating parse error)

This way I suggest we accept dateparser without adding our layer on top of it, but file bugs about StrictMode not being handled well and also with all examples where we could find dateparser not reporting the error. Does it make sense in your view? Could you please file the issues? If yes - I would suggest to add links to the issues to formats.txt .

Some nits about the changes themselves:

tox tests become broken; could you please fix?
could we please have "1_week_ago" instead of "1 week ago" in tidrange help output? if we are to use dates with spaces it requires quoting, so either we should add proper quoting to the help, or use the format that does not depend on whether timestamp is quoted or not. In the help I would suggest to use underscores without quoting and users who like spaces can get that information from date-reference.
could we please rename tidrange-formats.txt into something else? There is nothing tidrange specific in there - it is only about a tid and timestamp, not tid range. I suggest something like tid-time-format.txt. Is it ok?
for parse_tid I suggest we remove empty -> empty from there. This function should onlyparse a tid or timestamp, and if input is not valid - it should be an error. Additional reason to get this right is because from API-level point of view parse_tid is exported. Since tid=None make sense only for when iterating a range I would sugges to move the check for emptiness into parse_tidrange instead. Is it ok with you?
for parse_tid I would suggest not to raise TidRangeInvalid and raise tid-specific error, e.g. TidInvalid instead. The reason is - if we let TidRangeInvalid to be raised from parse_tid, and let it be propagagted through parse_tidrange and up to the caller - the error message would contain only one invalid tid instead of whole input, which would be confusing to user, e.g.

$ zodbanalyze foo..bar
E: invalid tid range: foo         # should be invalid tid range: foo..bar   or  invalid tid: foo

do you agree?

I would suggest to rework parse_tid a bit for clarity - to avoid e.g. double fromhex calls etc, e.g. this way:

def parse_tid(tid_string, raw_only=False):
    assert isinstance(tid_string, (str, bytes))

    # If it "looks like a TID", don't try to parse it as time,
    # because parsing is slow.
    if len(tid_string) == 16:
        try:
            return fromhex(tid_string)
        except ValueError:
            pass

    if raw_only:
        # either it was not 16-char string or hex decoding failed
        raise TidInvalid(tid_string)

    # preprocess to support `1.day.ago` style formats like git log does.
    if "ago" in tid_string:
        tid_string = tid_string.replace(".", " ").replace("_", " ")
    parsed_time = dateparser.parse(
        tid_string,
        settings={
            'TO_TIMEZONE': 'UTC',
            'RETURN_AS_TIMEZONE_AWARE': True
        })

    if not parsed_time:
        # parsing as date failed
        raise TidInvalid(tid_string)

    # build a ZODB.TimeStamp to convert as a TID
    return TimeStamp(
            parsed_time.year,
            parsed_time.month,
            parsed_time.day,
            parsed_time.hour,
            parsed_time.minute,
            parsed_time.second + parsed_time.microsecond / 1000000.).raw()

in test_parse_tid_time_format you don't use reference_time at all. This means that from test-formats.txt only reference_tid and input_time are checked in between each other. I was already saying that we could drop the tid itself from test-formats.txt (as tid and reference time are 1-1 related and maintaining human-readable input test vector is much easier when verifying time-time compared to manually verifying tid-time), but if you want to keep the tid in there we should not anyway ignore reference_time. In other words with reference_time,reference_tid,input_time that test should verify that both:

1) reference_tid corresponds to reference time, and
2) input_time corresponds to reference_tid

since for now we only have 2), there could be an error in test-formats.txt in reference time which will go unnoticed.

Are you ok to rework it?

also is it ok in your view if we leave reference to python-specific dateparser in tidrange help? I mean this would not apply out-of-the box to zodbtools/go, though it also should not be a blocker for us here. If we could find some wording that is expressive enough and general, maybe that would work better? (this point should not prevent us to merge the patch, though if you have some thoughts or possible improvements it should be good).

Thanks again for the update. It looks like the changes are converging to the point when we could land them into zodbtools, which would be good and handy for people. Thanks again for your work, and looking forward to hear from you again,

Kirill

added 17 commits

c1004a3a...f0399d50 - 15 commits from branch nexedi:master
bace418b - test: test for tidrange with tids
5ea85b8a - tidrange: supports giving range as time

Compare with previous version

@kirr thanks a lot for this review.

About dateparser not detecting invalid input - I've tried dateparser master and for some of your example invalid inputs it does detect the badness: ...

Oh thanks, you are right, thanks for double checking this.

I had in my notes:

ERROR timespec: 2009-06-01T10:00:00:+09:00 expected time: 2009-06-01T01:00:00Z parsed time: 2009-06-07T04:00:00Z

but this is from several days ago already and I don't remember exactly how I did. This morning, I observed the same behaviour as you - ie. parse error returning None. I'm not able to reproduce what I did wrong, but I see the parsed time ( 2009-06-07T04:00:00Z) is the input from line above. I guess my code was wrong when I took this note. I suspect something like try: except: and still having the parsed output from previous line. Definitely something was wrong in my proto.py script.

So I have to look at this again ( and also fix these notes). Apparently dateparser works well enough ( without the need to enable strict parsing mode ). For now, I filed https://github.com/scrapinghub/dateparser/issues/491 that's not a strict parsing mode problem, but an error I found while working on that prototype.

...

tox tests become broken; could you please fix?

I already addressed this part by rebasing on master and added test dependencies to tox.ini, tests are now passing for me.

...

in test_parse_tid_time_format you don't use reference_time

true, but it's nice to have it, as it makes better error message when test fail, so I agree that we should verify both equalities as you suggested.

The rest, I agree with everything, thanks a lot.

maybe the

def parse_tid(tid_string, raw_only=False):
    assert isinstance(tid_string, (str, bytes))

could become

def parse_tid(tid_string, raw_only=False):
    if not isinstance(tid_string, (str, bytes)):
        raise TypeError("tid_string must be string or bytes")

I'll go back working on this tomorrow or the day after.

@jerome, thanks for feedback, for fixing tox and for filing dateparser issue; glad my comments were helpful. Yes - that's why we have review. From the go book:

Programs today are far larger and more complex than in Wilkes’s time, of course, and a great deal of effort has been spent on techniques to make this complexity manageable. Two techniques in particular stand out for their effectiveness. The first is routine peer review of programs before they are deployed. The second, the subject of this chapter, is testing.

About test_parse_tid_time_format and reference_time - please don't get me wrong - I was not against using reference time there - what I was saying was quite the contrary - we can drop reference tid from input as we can reliably convert reference time to reference tid with just ZODB. This way we can keep in input.txt only reference time and human-readable-time, and still in the test have reference time, reference tid and input time. I think editing input.txt in terms of time only is easier to manage as it is easier to verify time-time correspondence compared to time-tid correcpondence without using a tool. From this point of view we can even move the functionality to parse time into its own package (under zodbtools) as it comes ZODB-independent - and use it in parse_tid.

That said it is ok for me if you leave reference tid in input.txt, however in the test please verify all fields to be corresponding to each other (and btw we probably should also have "3" reference_time <-> input_time).

About assert vs if not ...: raise TypeError: the reason I've added this is that the lack of static typing for function signatures hurts projects in my experience - it makes it harder to understand what a function accepts and what it returns. In other words at some points too much dynamic typing hurts the codebase and makes it less understandable. Ideally I would just add a type for tid, e.g.

def parse_tid(str tid, bool raw_only=True) ZODB.tid

if it was Cython, or

def parse_tid(tid: str, raw_only=True: bool) -> bytes

with mypy/python3, but while still being py2/py3 we have no that support.

Personally I think assert has more signal/noise to the programmer compared to two-lines if/raise (imagine if it was multiple parameters).

That said I do not have a strong feeling here for now and we can proceed either way.

Looking forward for updates, but please note that regarding zodbtools there is no urgency - so please take your time if you need to doublecheck something or if there is a pressure on other higher-priority projects.

Kirill

mentioned in commit jerome/zodbtools@a37151c9

added 10 commits

8f1dc3d5 - zodbtools v0.0.0.dev7
41ebc370 - test: test for tidrange with tids
a077a9cf - tidrange: supports giving range as time
1343136d - fixup! tidrange: supports giving range as time
877b973c - test_tidrange: code format
77fa2e4b - utils: move test for emptyness in parse_tid
e838df83 - tidrange: different exceptions for TidRangeInvalid/TidInvalid
a37151c9 - zodbtools: simplify parse_tid
42dc7178 - test_tidrange: also check that reference_tid matches reference_time
88531114 - help: do not mention dateparser in zodb help tidrange

Compare with previous version

marked as a Work In Progress from jerome/zodbtools@1343136d

Thanks again for feedback here @kirr

Update:

adjust the text from my notes in that "work" branch to remove the wrong output I took when my script had a bug
use 1_week_ago in zodb help
rename test input to tid-time-format.txt, I also like this naming
moved the "is empty" test in parse_tidrange
introduce a TidInvalid along with the TidRangeInvalid, also made them subclass ValueError instead of Exception ( Raised when an operation or function receives an argument that has the right type but an inappropriate value sounds appropriate here, isn't it ? ). Extend a bit test suite to check error cases. parse_tid(empty) is such an error.
I applied your suggested edits to "remove double fromhex calls etc"
About the test for time, I was thinking that if we omit the reference_tid it looks a bit like a test that is testing only the parser. I tested that the 3 are equals, but maybe that's OK if we remove reference_tid from input text and adjust the test as: python def test_parse_tid_time_format(fake_time, reference_time, input_time): assert ashex(parse_tid(reference_time)) == ashex(parse_tid(input_time)) Is the later your preferred way ? I don't have strong preference.
I updated tidrange help to no longer mention python dateparser.

Please take another look. I left individual commits for each change, but before merge I'd like to squash them together.

One thing is that the test for invalid TID are slow, because dateparser is slow on invalid input. time tox goes from 0m7.122s on master to 0m26.490s on this branch (these times are the second run when running tox twice in a row).

One more note about the types. I'm interested in types these days, especially for the IDE integration ( I'm using theia these days ). One thing that works today in python 2 and python 3 is:

def parse_tid(tid_string, raw_only=False):
    # type: (str, bool) -> bytes
    """Try to parse `tid_string` as a time and returns the

at least it works for jedi's autocompletions ( when doing parse_tid(tidmax). it completes with bytes methods). According to its documentation, mypy also supports this syntax (it's also defined in a PEP)

I saw ( https://twitter.com/gvanrossum/status/1085718143039725568 ) that mypy recently got support to compiling to C extensions, this seems interesting (also Litteral, which is one thing that makes typescript so nice ). I have not tried yet, but it looks great.

[edit to fix a typo]

added 1 commit

28e757c4 - test_tidrange: remove unused import

Compare with previous version

@jerome, thanks for the update. The changes look good and I think we are close to be ready to merge. I would say we should merge with current state, but unfortunately I see test failures both with plain py.test run and with under tox for all varants (details attached at the end). Could you please see what is going on?

About dateparser being slow on invalid input - could you please file a bug for dateparser upstream? We can accept the slowness for now, but it would be good at least for dateparser developers to know about that they have the huge speed issue.

About reference_tid: I was meaning the functionality to parse a human-readable date is separate functionality and we could be having and testing it without tids and ZODB specifity at all. That is the job of dateparser, yes, but we anyway prepare the input for it a bit - e.g. we change . and _ to and only then feed it to dateparser.

What I was meaning is to have a separate package/function dedicated to parsing such dates, e.g. humantime.parse() and internally there do all the preparations and interactions with dateparser. In humantime_test we would also use only reference_time and human_time without any mentioning of tids.

Then in parse_tid we would use only humantime.parse() call and in parse_tid tests we would reference humantime/testdata/time-formats.txt and prepare time-tid tests table based on that.

This way we'll have a module to parse human dates that we know is interoperable in between all our utilities, and also in between humantime on Go side, where inside we'll be using e.g. when, but with humantime we will be sure it is interoperable with our python humantime.

If you like it - feel free to rework it this way, however if it creates another time blocker I suggest we merge your current changes as is for now and just put a corresponding TODO.

Thanks for notes on typing. I suggest let's then use what tools understand, i.e. the comment way for now since we are going to support python2. BTW Cython also seems to be supporting PEP 484: https://github.com/cython/cython/issues/1672. However with mypyc it seems python is finally getting into being compiled language (it was already compiled language, but it was compiling into bytecode, and now into C with the help of static type) - lots of overlapping with cython here...

Thanks again for the update and looking forward to tests fix and the final patch,
Kirill

---- 8< ---- (test failures)

(neo) (z-dev) (g.env) kirr@deco:~/src/wendelin/z/zodbtools$ py.test
==================================================== test session starts ====================================================
platform linux2 -- Python 2.7.14, pytest-4.1.0, py-1.7.0, pluggy-0.8.0
rootdir: /home/kirr/src/wendelin/z/zodbtools, inifile:
plugins: threadleak-0.2.0
collected 35 items                                                                                                          

zodbtools/test/test_analyze.py .                                                                                      [  2%]
zodbtools/test/test_commit.py ..                                                                                      [  8%]
zodbtools/test/test_dump.py ...                                                                                       [ 17%]
zodbtools/test/test_tidrange.py ......................FFFFFFF                                                         [100%]

========================================================= FAILURES ==========================================================
______________________ test_parse_tid_time_format[2009-08-29T04:00:00Z-0380559000000000-6am yesterday] ______________________

fake_time = None, reference_time = '2009-08-29T04:00:00Z', reference_tid = '0380559000000000', input_time = '6am yesterday'

    @pytest.mark.parametrize("reference_time,reference_tid,input_time",
                             test_parameters)
    def test_parse_tid_time_format(fake_time, reference_time, reference_tid,
                                   input_time):
>       assert reference_tid == ashex(parse_tid(input_time))
E       AssertionError: assert '0380559000000000' == '0380551800000000'
E         - 0380559000000000
E         ?       ^        -
E         + 0380551800000000
E         ?       ^^

zodbtools/test/test_tidrange.py:110: AssertionError
______________________ test_parse_tid_time_format[2009-08-29T16:00:00Z-0380586000000000-6pm yesterday] ______________________

fake_time = None, reference_time = '2009-08-29T16:00:00Z', reference_tid = '0380586000000000', input_time = '6pm yesterday'

    @pytest.mark.parametrize("reference_time,reference_tid,input_time",
                             test_parameters)
    def test_parse_tid_time_format(fake_time, reference_time, reference_tid,
                                   input_time):
>       assert reference_tid == ashex(parse_tid(input_time))
E       AssertionError: assert '0380586000000000' == '038057e800000000'
E         - 0380586000000000
E         ?       -        -
E         + 038057e800000000
E         ?      ++

zodbtools/test/test_tidrange.py:110: AssertionError
__________________________ test_parse_tid_time_format[2009-08-30T01:00:00Z-03805a7c00000000-3:00] ___________________________

fake_time = None, reference_time = '2009-08-30T01:00:00Z', reference_tid = '03805a7c00000000', input_time = '3:00'

    @pytest.mark.parametrize("reference_time,reference_tid,input_time",
                             test_parameters)
    def test_parse_tid_time_format(fake_time, reference_time, reference_tid,
                                   input_time):
>       assert reference_tid == ashex(parse_tid(input_time))
E       AssertionError: assert '03805a7c00000000' == '03805a0400000000'
E         - 03805a7c00000000
E         ?       ^^
E         + 03805a0400000000
E         ?       ^^

zodbtools/test/test_tidrange.py:110: AssertionError
__________________________ test_parse_tid_time_format[2009-08-30T13:00:00Z-03805d4c00000000-15:00] __________________________

fake_time = None, reference_time = '2009-08-30T13:00:00Z', reference_tid = '03805d4c00000000', input_time = '15:00'

    @pytest.mark.parametrize("reference_time,reference_tid,input_time",
                             test_parameters)
    def test_parse_tid_time_format(fake_time, reference_time, reference_tid,
                                   input_time):
>       assert reference_tid == ashex(parse_tid(input_time))
E       AssertionError: assert '03805d4c00000000' == '03805cd400000000'
E         - 03805d4c00000000
E         ?        -
E         + 03805cd400000000
E         ?      +

zodbtools/test/test_tidrange.py:110: AssertionError
_______________________ test_parse_tid_time_format[2009-08-30T10:00:00Z-03805c9800000000-noon today] ________________________

fake_time = None, reference_time = '2009-08-30T10:00:00Z', reference_tid = '03805c9800000000', input_time = 'noon today'

    @pytest.mark.parametrize("reference_time,reference_tid,input_time",
                             test_parameters)
    def test_parse_tid_time_format(fake_time, reference_time, reference_tid,
                                   input_time):
>       assert reference_tid == ashex(parse_tid(input_time))
E       AssertionError: assert '03805c9800000000' == '03805c2000000000'
E         - 03805c9800000000
E         ?       ^^
E         + 03805c2000000000
E         ?       ^        +

zodbtools/test/test_tidrange.py:110: AssertionError
_____________________ test_parse_tid_time_format[2009-08-29T10:00:00Z-038056f800000000-noon yesterday] ______________________

fake_time = None, reference_time = '2009-08-29T10:00:00Z', reference_tid = '038056f800000000', input_time = 'noon yesterday'

    @pytest.mark.parametrize("reference_time,reference_tid,input_time",
                             test_parameters)
    def test_parse_tid_time_format(fake_time, reference_time, reference_tid,
                                   input_time):
>       assert reference_tid == ashex(parse_tid(input_time))
E       AssertionError: assert '038056f800000000' == '0380568000000000'
E         - 038056f800000000
E         ?       -
E         + 0380568000000000
E         ?                +

zodbtools/test/test_tidrange.py:110: AssertionError
____________________ test_parse_tid_time_format[2009-06-07T04:00:00Z-037e7d1000000000-6AM, June 7, 2009] ____________________

fake_time = None, reference_time = '2009-06-07T04:00:00Z', reference_tid = '037e7d1000000000'
input_time = '6AM, June 7, 2009'

    @pytest.mark.parametrize("reference_time,reference_tid,input_time",
                             test_parameters)
    def test_parse_tid_time_format(fake_time, reference_time, reference_tid,
                                   input_time):
>       assert reference_tid == ashex(parse_tid(input_time))
E       AssertionError: assert '037e7d1000000000' == '037e7c9800000000'
E         - 037e7d1000000000
E         ?      ^^        -
E         + 037e7c9800000000
E         ?      ^^^

zodbtools/test/test_tidrange.py:110: AssertionError
============================================ 7 failed, 28 passed in 3.02 seconds ============================================

(neo) (z-dev) (g.env) kirr@deco:~/src/wendelin/z/zodbtools$ tox
...
ERROR:   py27-ZODB3: commands failed
ERROR:   py27-ZODB4: commands failed
ERROR:   py27-ZODB5: commands failed

Oh thanks for being patient with me :)

I can already say that test failure seem to be a problem with the way we fake the timezone . I only tried variations of $TZ but did not consider it could read this elsewhere. On another machine I see:


zodbtools/test/test_tidrange.py:110: AssertionError
______________________ test_parse_tid_time_format[2009-06-07T04:00:00Z-037e7d1000000000-6AM, June 7, 2009] ______________________

fake_time = None, reference_time = '2009-06-07T04:00:00Z', reference_tid = '037e7d1000000000', input_time = '6AM, June 7, 2009'

    @pytest.mark.parametrize("reference_time,reference_tid,input_time",
                             test_parameters)
    def test_parse_tid_time_format(fake_time, reference_time, reference_tid,
                                   input_time):
>       assert reference_tid == ashex(parse_tid(input_time))
E       AssertionError: assert '037e7d1000000000' == '037e7b6c00000000'
E         - 037e7d1000000000
E         ?      ^^        -
E         + 037e7b6c00000000
E         ?      ^^^

and if I change sudo dpkg-reconfigure tzdata to your timezone I have same difference as you, so this is due to the way we fake machine timezone. I'll look into this shortly.

About dateparser being slow on invalid input

I see there is https://github.com/scrapinghub/dateparser/issues/426 and the corresponding fixes, that are not yet part of a released version really speed things up. The first parse of invalid string is still a bit slow, but subsequent are really fast. When using master branch of https://github.com/scrapinghub/dateparser/ , time tox goes down from 30 to 20 seconds. I don't feel it would be of any help to file another "it's slow" issue there. They seem to be working on performance improvements already.

... humantime ...

I was also thinking that something to parse human dates would be nice to have, not only for time but also for time range. It would be nice if in ERP5 we could search for "invoices from last month". I prefer that we first finish this simple version and later extract it in a package.

@jerome, thanks for feedback. I see about timezones - it is strange, if I recall correctly I was running the tests on previous version of your patch and there were no problems, but I do not remember for sure. Anyway good that you have a way to reproduce the problem, that should help to get it fixed. Thanks also for dateparser links - I agree there is no need to file another bug about the slowness, but I filed the issue asking them to release a new version of dateparser: https://github.com/scrapinghub/dateparser/issues/494.

Re humantime: I agree.

added 4 commits

28e757c4...f0399d50 - 2 commits from branch nexedi:master
3e35453c - test: test for tidrange with tids
81b566c4 - tidrange: supports giving range as time

Compare with previous version

Thanks for filing the issue asking for new release. So I suggest we leave it like this that tests are a bit slow but will be 50% faster once this new dateparser will be used. What seem to be slow is initialisation of locale data, to make tests a few seconds faster we could probably "cheat" and disable most locales, but I feel it's not worth the effort.

You were right tests were working for previous version of the patch. The problem appeared that when I introduced a new test for invalid tid ( test_parse_tid ). This test does not use the fake_time fixture (because it does not need it as it test anything related to time), but it was running before the date test and caused "what's the local timezone ?" information to be cached by tzlocal. To me, what that revealed is that tests where not isolated because that fake_time fixture was not doing its job correctly. I changed it to also call tzlocal.reload_localzone() and this fixed the test for me.

I also squashed together all intermediate commits.

unmarked as a Work In Progress

mentioned in commit kirr/zodbtools@4037002c

merged

@jerome, thanks for the fixup and explanation. I agree we should not delve into optimizing test runs for now. The changes look good - let's merge them.

Are you interested in releasing new zodbtools version with your improvements? I mean are you interested to make the release by yourself?

Also, what is the status of your go-side changes? Since you were playing with making the same functionality on go side, it would be nice to land it into zodbtools/go too. However I would totally understand if chances are that there is no enough interest to finish go bits, or if working on go side is not a priority for you. Just please share what you have in mind.

Thanks again for your patches. It is great that we can now use human-readable relative dates like 1_week_ago or yesterday with zodbtools.

Kirill

(merged as 4037002c)

Thanks @kirr , for all the valuable feedback and for merging.

Are you interested in releasing new zodbtools version with your improvements? I mean are you interested to make the release by yourself?

I can do a 0.0.0.dev8 next week ( I'll be busy tomorrow and Friday). I will follow the procedure used for wendelin.core - I see we did not tag the 0.0.0.dev*, is it intentional ? ( because the -dev ?)

Also, what is the status of your go-side changes?

Yes it would be nice if I finish the go tutorial and then finish the go changes one day, but this will not be in the near future. If you or someone else needs the change in the go version, don't wait for me.

I also have somewhere the beginning of some work on making all this python3 compatible, but there are several places in the code where I'm not sure if it should be bytes or str, also if I understand correctly python3 cannot open filestorage created with python 2. This python 3 really not advanced and if I have no idea when I'll work on this. I'll cleanup a bit and share the current state, but we should not wait for this to make a release.

@jerome, thanks for feedback. There is no hurry with the release, so if you can do it next week - that's fine. I do not remember why I did not tag previous releases, probably because initially releases were too frequent (compared to the rate of change). Feel free to either continue this way or to add a tag (and BTW, it is never late to add tags for older releases, so probably we should do that too).

For go - there is no urgency at all - for now we can live with just zodbtools/py supporting human dates and if eventually you can come up with zodbtool/go support - that would be good. So please take your time to do the tutorial and maybe other learnings. For me it was imporant to know whether on your side things are planned to be finished, even if eventually, or not planned at all. I will thus keep in mind you have this planned but it won't come up in the near future.

It would be good if we can fix Python3 support. Yes, ZODB/py3 broke data compatibility with ZODB/py2, which is a huge mistake in my view. However we can work it around with generating two test databases - one with ZODB/py2 and one with ZODB/py3 and using a particular test database depending on which python version tests are run with. (We can also cheat a bit and dynamically patch FileStorage header in the test database so that ZODB/py3 could open it. However this requires us to be careful to use only types that are pickled/unpickled in the same way irregargless of python version.)

Yes, it is not always clear whether something should be bytes or str, or even if it s clear, it is too inconvenient to always do back-forth conversions. Practically speaking, I would love to extend gpython support for this (gpython already fixed default string encoding to UTF-8) to automatically convert bytes <-> str with that encoding. I've looked at cpython codebase some time ago, and if I recall correctly - it is only 2 places that needs to be patched (constructor for bytes to use default encoding, and constructor for str). Unfortunately this patches should be done at C level and we cannot make them with easy way gpython works currently. See also https://www.nexedi.com/NXD-Presentation.Multicore.PyconFR.2018 (search for "they use UTF-8 strings instead") for a related presentation by Jean-Paul (the whole presentation is also useful to read). For the reference for pickles I've tried to document bytes <-> str relation with respect to pickle protocol version being used - see https://godoc.org/github.com/kisielk/og-rek for details.

We should not delay making a new release waiting for py3 stuff, however if you have at least some bits of py3 compatibility ready - don't hesitate to file merge-requests with the patches - my preference is not to wait for the one patch or bigger merge request for py3 compatibility to get fixed at once, and instead merge py3-compat bits as soon as we can (e.g. print function is obvious).

Thanks again for feedback and looking forward for updates from you. Please also don't hesitate to come to ask something about Go, if things are moot somewhere and it is better to clarify the picture.

Kirill

P.S. using the occassion I'd like to also say thanks for pytest support.

It would be good if we can fix Python3 support.

I pushed the current state "as is" in !12

Good morning Kirill

I can do a 0.0.0.dev8 next week ( I'll be busy tomorrow and Friday).

I did not forget, but could not find the time this week. I don't want to do this in a hurry and make a mistake.

@jerome, thanks for update. No worries - there is no urgency with zodbtools release, and it is indeed better to do the release not in a hurry.

Hello @kirr I finally prepared a release from current master and uploaded it on test pypi at https://test.pypi.org/project/zodbtools/

Unless you or others have objects, I'll push jerome/zodbtools@fbd81c00 to master and upload on real pypi.

@jerome, thanks, glad to hear from you. The release looks ok for me. I only have minor suggestions, that I've left on jerome/zodbtools@fbd81c00, but taking them into account should be optional as the relase already generally looks good. Please feel free to either make the actual release as is, or with the suggested amendments.

Thanks again,
Kirill

Thank you again @kirr I uploaded the release at https://pypi.org/project/zodbtools/0.0.0.dev8/ it is based on bcaf3984

@jerome, great! Thanks for caring about this release.

mentioned in merge request erp5!1053

nexedi / zodbtools

Support absolute and relative time in tidrange

Merged

Support absolute and relative time in tidrange

Check out, review, and merge locally

Merged