bpo-29995: re.escape() now escapes only special characters.

[serhiy-storchaka]

No description provided.

[mention-bot]

@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @tiran and @ezio-melotti to be potential reviewers.

[bz2]

Change looks good to me, as random third party that has written a function like this in the past.

Bunch of teeny inline comments, none of which should block merge.

The automated coverage report failure is bogus, seems to be percentage change from deleting code plus random noise.

[bz2]

For my own reference, re.VERBOSE does only look at ascii whitespace:

>>> re.compile("a \f\n\r\v\t\u3000", re.DEBUG|re.VERBOSE)
LITERAL 97
LITERAL 12288
re.compile('a \x0c\n\r\x0b\t\u3000', re.VERBOSE|re.DEBUG)

[serhiy-storchaka]

SPECIAL_CHARS and WHITESPACE are constants in the sre_parse module. WHITESPACE contains only ascii whitespace.

[bz2]

This redefines literal_chars to the same as it was before. Either delete, or split the test, or hoist to a suite level constant?

[serhiy-storchaka]

Oh, I copied this definition rather than move!

[bz2]

To "now escapes only regex special characters" just for clarity?

[serhiy-storchaka]

Done.

[bz2]

To literal_bytes = self.LITERAL_CHARS.encode() if hoisting above.

[serhiy-storchaka]

Hmm, this looks a good idea.

[bz2]

To escape(pattern) to match implementation naming?

[bz2]

Not sure what the style on naming for functions that take either str or bytes is, but can see argument for "...characters in string or bytes pattern".

[serhiy-storchaka]

I want to avoid the word "pattern". The argument of escape() is not a pattern, it is an arbitrary string, and escape() makes a pattern from it.

The name "string" is often used for text and byte strings in this module. It is the name of parameters in many functions and the name of the attribute in the match object.

[bitdancer]

The normal way to write this would be "escape special characters in pattern", since pattern is the formal argument name in the source. That is, I agree with your previous comment that string should be replaced by pattern. The existing docs even use 'pattern' in the body of the text, so I'm not sure how we ended up with 'string' as the formal argument name in the docs.

[serhiy-storchaka]

Opened bpo-30045 for renaming the parameter to "string".

[serhiy-storchaka]

In #1048 the string parameter name was replaced by pattern and it is referred as *pattern* in the description.

[bz2]

Changes look good to me.

[jaraco]

Mentioning bpo-29995 to create a link to it.

[jaraco]

This change is likely implicated in pypa/setuptools#1015.

[jaraco]

@serhiy-storchaka Is the intention that tests like these that are asserting a certain result from re.escape should adapt their expectations based on Python version?

[serhiy-storchaka]

I think that it would be better to not depend on exact result of re.escape. Either use re.escape for calculating the part of the expected result that is out of your full control:

assert get_pattern(l('foo/bar')) == l(re.escape('foo/bar') + r'\Z(?ms)')

Or test the pattern indirectly. For example, it should match 'foo/bar', but not 'foo/bar\n'.

Note that compiling the pattern r'foo/bar\Z(?ms)' emits a deprecation warning in 3.6+ since flags are not at the start of the expression. It is worth to rewrite get_pattern() until it become an error.