repr(regex) doesn't include actual regex

[ezio.melotti]

http://bugs.python.org/review/13592/diff/5455/Lib/test/test_re.py
File Lib/test/test_re.py (right):

http://bugs.python.org/review/13592/diff/5455/Lib/test/test_re.py#newcode928
Lib/test/test_re.py:928: self.assertEquals('re.compile("random pattern",
re.UNICODE)', repr(pattern))
3 comments:
1) assertEquals is deprecated, you should use assertEqual instead (without the
final 's').  If you run the tests with -Wd you should get a warning for this;
2) it should be assertEqual(actual, expected), not assertEqual(expected,
actual);
3) to improve readability and to avoid having more than 80 chars per line (see
PEP8), I'd suggest to go on a new line after the first argument of assertEqual,
e.g.:
self.assertEqual(re.compile('random pattern'),
                 "re.compile('random pattern', re.UNICODE)")

http://bugs.python.org/review/13592/diff/5455/Lib/test/test_re.py#newcode940
Lib/test/test_re.py:940: self.assertEquals('re.compile("random pattern",
re.IGNORECASE|re.DOTALL|re.UNICODE|re.VERBOSE)', repr(pattern))
I'd also add some tests that use the short constants names, e.g. re.I, re.S,
etc.
A test that uses inline flags would be good too (e.g. re.compile('(?i)foo').

http://bugs.python.org/review/13592/diff/5455/Modules/_sre.c
File Modules/_sre.c (right):

http://bugs.python.org/review/13592/diff/5455/Modules/_sre.c#newcode2693
Modules/_sre.c:2693: if (!append_string(list, "re.compile(\""))
Wouldn't this break if the pattern contains a " ?
Maybe using just "re.compile(" and appending the repr of obj->pattern is better.

[twouters]

http://bugs.python.org/review/13592/diff/5467/Lib/test/test_re.py
File Lib/test/test_re.py (right):

http://bugs.python.org/review/13592/diff/5467/Lib/test/test_re.py#newcode958
Lib/test/test_re.py:958: def test_repr_with_single_quotes_inside(self):
For good measure, add a test with both types of quotes.

http://bugs.python.org/review/13592/diff/5467/Modules/_sre.c
File Modules/_sre.c (right):

http://bugs.python.org/review/13592/diff/5467/Modules/_sre.c#newcode2603
Modules/_sre.c:2603: pattern_repr(PatternObject *pattern);
If you move the definition up (for example, alongside pattern_dealloc) you won't
need this forward declaration.

http://bugs.python.org/review/13592/diff/5467/Modules/_sre.c#newcode2647
Modules/_sre.c:2647: item = Py_BuildValue("U", string);
This should just use "s" instead of "U", in Python 3.

http://bugs.python.org/review/13592/diff/5467/Modules/_sre.c#newcode2663
Modules/_sre.c:2663: separator = Py_BuildValue("U", string);
This should also use "s".

http://bugs.python.org/review/13592/diff/5467/Modules/_sre.c#newcode2678
Modules/_sre.c:2678: PatternFlag flags[] = {
Make this a (static) global array instead of an auto var.

http://bugs.python.org/review/13592/diff/5467/Modules/_sre.c#newcode2692
Modules/_sre.c:2692: if (!list)
Compare against NULL explicitly, instead of using !list. "if (list == NULL)'.

http://bugs.python.org/review/13592/diff/5467/Modules/_sre.c#newcode2695
Modules/_sre.c:2695: return NULL;
This leaks the reference to 'list'. The usual pattern here is to assign NULL to
all objects you might to deallocate in error paths, do XDECREFs in a separate
error path and use 'goto error' to jump there.

The same goes for almost all return statements below.

http://bugs.python.org/review/13592/diff/5467/Modules/_sre.c#newcode2700
Modules/_sre.c:2700: return NULL;
for pattern_repr, since you DECREF it after this call and this is the only case
in which you need to handle it, you probably want to DECREF it and then goto
error.

http://bugs.python.org/review/13592/diff/5467/Modules/_sre.c#newcode2703
Modules/_sre.c:2703: flag_items = PyList_New(0);
Since you can look at the flags and see instantly whether you're going to need
this, you can turn the PyList_Size() check below into a 'if obj->flags' check
right here, and skip the rest of the code if there are no flags. For
maintainability it's probably a good idea to define a new SRE_FLAG_ALLFLAGS or
such that is the bitwise-or of all possible flags, and add it to sre_constants.h
(in case the sre module grows uses of the flags int that *isn't* an actual
flag.) You can then write the quick check for flags as 'if (obj->flags &
SRE_FLAG_ALLFLAGS)'.