search

andreasvc / pyre2

Python wrapper for RE2
BSD 3-Clause "New" or "Revised" License
99 stars 33 forks source link

fix behavior with empty matches #27

Open andreasvc opened 3 years ago

andreasvc commented 3 years ago

Python 3.7 fixed bugs related to empty matches: https://bugs.python.org/issue1647489

sarnold commented 3 years ago

Okay, but I don't yet see anything pointing to this behavior change, or how using the patch you just merged exhibits the old behavior in ubuntu focal with python 3.9.6 but I get the new behavior in python 3.9.6 locally. WTH??

Anyway, I got bounced from the original "this should match perl's behavior" discussion from 2007:

https://bugs.python.org/issue1647489

to a closed bug and then a closed PR with no apparent fix:

https://github.com/python/cpython/pull/4678

to another (later) closed bug on Python 3.7, Python 3.6, Python 2.7:

https://bugs.python.org/issue25054

and now too I'm tired to bisect and I think I'd rather push a few updates on another PR anyway.

andreasvc commented 3 years ago

It's an obscure issue. If Python cannot make up its mind about it, maybe we should just remove the test and document that the behavior is inconsistent.

sarnold commented 3 years ago

I have this one: https://github.com/sarnold/pyre2/pull/9 and everything is "fine" except coverage.py chokes with a parse error on the first cimport it sees. I tried the exact same config on the datrie extension and coverage works fine, so I'm not sure if it's a coverage.py bug or the extension code itself. It would be really nice to have some test coverage, but I'm not sue exactly what needs "fixing"...

sarnold commented 3 years ago

Oh, I also split out 2 problematic tests so they can be deselected/marked with pytest. Right now there are 2 places where the None vs '' test fails with the patch applied (which are accounted for in the workflows and tox.ini).

sarnold commented 3 years ago

Finally, this will fail hard in python 3.10: https://github.com/borgbackup/borg/issues/5683

Fixing that ^^ and removing PY2 support should help getting the test coverage enabled.