moving _repr_fixups list from doctest/parsing.py header to method do_fixup

[seblabbe]

As a follow-up of #33588 where we suggested in comment:10 to move out of context _repr_fixups list from the header of doctest/parsing.py to the method do_fixup.

CC: @fchapoton @jhpalmieri @kiwifb

Component: doctest framework

Author: Sébastien Labbé

Branch/Commit: 5b539c5

Reviewer: John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/33680

[seblabbe]

Dependencies: #33588

[seblabbe]

Commit: 3ae51eb

[seblabbe]

New commits:

d175231	33588: adding try_fixup method in the SageOutputChecker
a67986c	33588: continue to compare the non-floats part of doctests when tolerance is given
0274a6a	33588: using if-else is better at showing the logic structure of the cases
bcca553	33588: renamed try_fixup -> do_fixup because looks better like this
4a4cd4b	33588:OUTPUT -> OUTPUT:
3ae51eb	33680: moving _repr_fixups from module header to method do_fixup

[seblabbe]

Branch: u/slabbe/33680

[sagetrac-git]

Changed commit from 3ae51eb to 3bc14de

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

11f7a34	33588: adding do_fixup method in the SageOutputChecker
1d337e4	33588: continue to compare the non-floats part of doctests when tolerance is given
79071f5	33588: using if-else is better at showing the logic structure of the cases
2afb472	33588: raw-string for documentation of check_output
3bc14de	33680: moving _repr_fixups from module header to method do_fixup

[jhpalmieri]

comment:7

Seems to need rebasing.

[jhpalmieri]

comment:10

Probably due to #34533?

[seblabbe]

Changed dependencies from #33588 to none

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

433e3e4

33680: solving conflict

[sagetrac-git]

Changed commit from 3bc14de to 433e3e4

[seblabbe]

comment:13

I solved the conflict. The interest in the current ticket is that it is more readable and testable when fixing an issue. For instance, new doctests could be added in the do_fixup method for the newly added fixup introduced in #34533 (I let someone else do it). Need review!

[jhpalmieri]

comment:14

As an overarching issue, I don't understand why so many regular expressions are defined at the top level in parsing.py. For example, optional_regex is used just once in the file, so why not define it and re.compile it right before it's used? I tried moving that definition inside the function parse_optional_tags and did some timing with ./sage -tp src/sage/misc: there was no difference from before the change to after.

Anyway, for this ticket, should we move glpk_simplex_warning_regex etc. to the method do_fixup? Again, I see no changes in the timing, although I have only run very limited tests.

[jhpalmieri]

comment:15

I like the change, in any case.

[jhpalmieri]

comment:16

I can push a change that moves all of the regular expressions out of the top level, if you would like.

[seblabbe]

comment:17

Replying to John Palmieri:

As an overarching issue, I don't understand why so many regular expressions are defined at the top level in parsing.py. For example, optional_regex is used just once in the file, so why not define it and re.compile it right before it's used?

I wonder if the reason is the following. Even if it is used only once in the file, the method can be called many times (for instance when doctesting the entire sage library), so that the compilation of the regex will be done only once and not thousands of times. What do you think?

[jhpalmieri]

comment:18

Replying to Sébastien Labbé:

Replying to John Palmieri:

As an overarching issue, I don't understand why so many regular expressions are defined at the top level in parsing.py. For example, optional_regex is used just once in the file, so why not define it and re.compile it right before it's used?

I wonder if the reason is the following. Even if it is used only once in the file, the method can be called many times (for instance when doctesting the entire sage library), so that the compilation of the regex will be done only once and not thousands of times. What do you think?

That's possible, but I didn't see any difference in timings. See https://stackoverflow.com/questions/452104/is-it-worth-using-pythons-re-compile/452143#452143 for a little analysis that reaches the conclusion that it doesn't save time to compile. I found the discussion at https://stackoverflow.com/questions/452104/is-it-worth-using-pythons-re-compile helpful in general. It seems that Python internally compiles and caches regular expressions anyway, so this shouldn't be saving us time, and in my observations it isn't. So I think we can make the decision based on what makes the code most readable.

See also the note here: https://docs.python.org/3/library/re.html#re.compile. This is all talking about compiling vs. not, but I think the same applies to where the compile statements are in the code.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

8a56793	33680: moving regex from header module to methods
5b539c5	33680: fixed a typo in the code: g -> got

[sagetrac-git]

Changed commit from 433e3e4 to 5b539c5

[seblabbe]

comment:20

I agree, I just did a commit doing that.

[jhpalmieri]

comment:21

Looks good to me.

[jhpalmieri]

Reviewer: John Palmieri

[vbraun]

Changed branch from u/slabbe/33680 to 5b539c5