A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I checked correctness of the return expression replacement, using test_cases above, with

for s in test_cases:    (pat(s) is not None) is (s.strip() in ('', '#'))

and all are True.

I checked correctness of the return expression replacement, using test_cases above, with

Ok, thank you, but thistest_cases is too small a sample and therefore I have supplemented my script with the following checks:

importstringimportrandomfromitertoolsimportproduct# First Checks:forcaseintest_cases:assertis_line_junk_regex(case)==is_line_junk_no_regex(case),f"Inconsistent behavior for{repr(case)}"defrandom_string(length=10):chars=string.printable+" "*5+"#"*5return"".join(random.choice(chars)for_inrange(length))# Second Checks:for_inrange(10000):case=random_string(random.randint(0,20))assertis_line_junk_regex(case)==is_line_junk_no_regex(case),f"Inconsistent behavior for{repr(case)}"# Third Checks:chars=" #\tabc"forlengthinrange(6):forcaseinmap("".join,product(chars,repeat=length)):assertis_line_junk_regex(case)==is_line_junk_no_regex(case),f"Inconsistent behavior for{repr(case)}"

Then I checked the tests inLib/test/test_difflib.py and everything finished well 👍

I have made the requested changes; please review again

Thanks for making the requested changes!

@terryjreedy,@AA-Turner: please review the changes made to this pull request.

Since we're improving performance, we don't want to recomputeline.strip()

Sorry, I suspected this but hoped that there was a cache mechanism here :)

I checked correctness of the return expression replacement, using test_cases above, with

forsintest_cases:    (pat(s)isnotNone)is (s.strip()in ('','#'))

and all are True.

I am going to be neutral an whether the basic change should be merged. If it is, I personally think the 'pat' parameter can be considered to be private and eliminated. But I would defer the back-compatibility to others, include 'encukou'. The only documented use of the function, marked as a constant, is to be passed as an argument. The argument is always passed on to SequenceMatcher, which calls it once with 1 argument, which would be a line.

@serhiy-storchaka

In any case, the relative performance depends on the input -- for a long string with a single space character at one end (e.g.'x'*10000+' ') the non-regex version is more than 10 times slower than the regex version. For length 100000 the difference is more than 100, and for length 1000000 -- more than 1000.

I tested your cases but didn't get the result you expected (maybe I did something wrong?)
I ran next script (accidentally mixed up the dividend and divisor, but it doesn't matter):

importreimporttimeitdefis_line_junk_regex(line,pat=re.compile(r"\s*(?:#\s*)?$").match):returnpat(line)isnotNonedefis_line_junk_no_regex(line):returnline.strip()==""orline.lstrip().rstrip()=="#"defbenchmark(func,cases,n=1_000):returntimeit.timeit(lambda: [func(line)forlineincases],number=n)lengths= [10_000,100_000,1_000_000]forlengthinlengths:test_string="x"*length+" "regex_time=benchmark(is_line_junk_regex,test_string)no_regex_time=benchmark(is_line_junk_no_regex,test_string)print(f"String length:{length}")print(f"Regex version:{regex_time:.6f} seconds")print(f"Non-regex version:{no_regex_time:.6f} seconds")print(f"Speedup:{no_regex_time/regex_time:.2f}x\n")

and got next results:

$./python -B bench.pyString length: 10000Regex version: 4.220341 secondsNon-regex version: 1.983180 secondsSpeedup: 0.47xString length: 100000Regex version: 42.698992 secondsNon-regex version: 20.897327 secondsSpeedup: 0.49xString length: 1000000Regex version: 516.844790 secondsNon-regex version: 262.632697 secondsSpeedup: 0.51x

Aren't you iterating over a single character instead of a full line:

[func(line)forlineincases]

You should pass[test_string] instead of justtest_string (sorry if I'm wrong, I'm tired)

I was the one who originally reqested restoringpat, but I do find Terry's argument compelling, and I wouldn't oppose removingpat.

I think we should restore this PR to the simpler previous version (the differences micro-benchmarks between string comparisons are so small they will be swallowed by noise). The class-levelre import should be resolved before merge, though.

A

@tim-one Do you have a reason to want to keep the regex?

Can't say I much care, but I don't get it. What's the point? It's dubious on the face of it to build an entirely new string object just to do simple lexical analysis. If this is some "micro-optimizztion" thing, instead of

stripped=line.strip()returnstripped==''orstripped=='#'

try

returnline.strip()in'#'# empty or single '#'

?

Can't say I much care, but I don't get it. What's the point? It's dubious on the face of it to build an entirely new string object just to do simple lexical analysis. If this is some "micro-optimizztion" thing, instead of

stripped=line.strip()returnstripped==''orstripped=='#'

try

returnline.strip()in'#'# empty or single '#'

?

that's how it was originally :)

I'll probably return it so as not to overcomplicate things

@rhettinger

@tim-one Do you have a reason to want to keep the regex?

Tim doesn't seem to mind

I don't know who I can ping but it seems like there are more suggestions