Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bpo-38208: Simplify string.Template by using __init_subclass__(). #16256

Merged
merged 3 commits into from Oct 21, 2019
Merged

bpo-38208: Simplify string.Template by using __init_subclass__(). #16256

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce02c9b
bpo-38208: Simplify string.Template by using __init_subclass__().
serhiy-storchaka Sep 18, 2019
606a4da
Remove unrelated changes.
serhiy-storchaka Sep 18, 2019
68d895b
Address review comments.
serhiy-storchaka Sep 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 4 additions & 5 deletions Lib/dataclasses.py
View file
Open in desktop
Expand Up @@ -199,11 +199,7 @@ def __repr__(self):
# https://bugs.python.org/issue33453 for details.
_MODULE_IDENTIFIER_RE = re.compile(r'^(?:\s*(\w+)\s*\.)?\s*(\w+)')

class _InitVarMeta(type):
def __getitem__(self, params):
return InitVar(params)

class InitVar(metaclass=_InitVarMeta):
class InitVar:
__slots__ = ('type', )

def __init__(self, type):
Expand All @@ -212,6 +208,9 @@ def __init__(self, type):
def __repr__(self):
return f'dataclasses.InitVar[{self.type.__name__}]'

def __class_getitem__(cls, params):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes aren't related to the title of this PR. Please either split it out into a separate PR, or retitle this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes. They are from other issue.

return InitVar(params)


# Instances of Field are only ever created from within this module,
# and only from the field() function, although Field instances are
Expand Down
45 changes: 21 additions & 24 deletions Lib/string.py
View file
Open in desktop
Expand Up @@ -54,30 +54,7 @@ def capwords(s, sep=None):

_sentinel_dict = {}

class _TemplateMetaclass(type):
pattern = r"""
%(delim)s(?:
(?P<escaped>%(delim)s) | # Escape sequence of two delimiters
(?P<named>%(id)s) | # delimiter and a Python identifier
{(?P<braced>%(bid)s)} | # delimiter and a braced identifier
(?P<invalid>) # Other ill-formed delimiter exprs
)
"""

def __init__(cls, name, bases, dct):
super(_TemplateMetaclass, cls).__init__(name, bases, dct)
if 'pattern' in dct:
pattern = cls.pattern
else:
pattern = _TemplateMetaclass.pattern % {
'delim' : _re.escape(cls.delimiter),
'id' : cls.idpattern,
'bid' : cls.braceidpattern or cls.idpattern,
}
cls.pattern = _re.compile(pattern, cls.flags | _re.VERBOSE)


class Template(metaclass=_TemplateMetaclass):
class Template:
"""A string class for supporting $-substitutions."""

delimiter = '$'
Expand All @@ -89,6 +66,25 @@ class Template(metaclass=_TemplateMetaclass):
braceidpattern = None
flags = _re.IGNORECASE

def __init_subclass__(cls):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a better approach, but if you're modernizing this then I have a few other suggestions.

super().__init_subclass__()
if 'pattern' in cls.__dict__:
pattern = cls.pattern
else:
pattern = r"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this pattern ought to be put at module scope, just above the class definition? The verbose representation might be more readable without all that leading whitespace.

%(delim)s(?:
(?P<escaped>%(delim)s) | # Escape sequence of two delimiters
(?P<named>%(id)s) | # delimiter and a Python identifier
{(?P<braced>%(bid)s)} | # delimiter and a braced identifier
(?P<invalid>) # Other ill-formed delimiter exprs
)
""" % {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too bad we can't convert this to a .format() string without breaking backward compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can. We can even convert it to f-string. I tried, but returned to the old format.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only problem with .format() and f-string is that we nee to duplicate literal braces. And the template/f-string will look strange, because braces have special meaning in regexpes and in new format.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With f-string it would look:

    def __init_subclass__(cls):
        super().__init_subclass__()
        if 'pattern' in cls.__dict__:
            pattern = cls.pattern
        else:
            delim = _re.escape(cls.delimiter)
            id = cls.idpattern
            bid = cls.braceidpattern or cls.idpattern
            pattern = fr"""
            {delim}(?:
              (?P<escaped>{delim})  |   # Escape sequence of two delimiters
              (?P<named>{id})       |   # delimiter and a Python identifier
              {{(?P<braced>{bid})}} |   # delimiter and a braced identifier
              (?P<invalid>)             # Other ill-formed delimiter exprs
            )
            """
        cls.pattern = _re.compile(pattern, cls.flags | _re.VERBOSE)

'delim' : _re.escape(cls.delimiter),
'id' : cls.idpattern,
'bid' : cls.braceidpattern or cls.idpattern,
}
cls.pattern = _re.compile(pattern, cls.flags | _re.VERBOSE)

def __init__(self, template):
self.template = template

Expand Down Expand Up @@ -146,6 +142,7 @@ def convert(mo):
self.pattern)
return self.pattern.sub(convert, self.template)

Template.__init_subclass__() # setup pattern
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a head scratcher, but I think the comment could be improved to make it more clear. What's really going on is that this is required to initialize pattern when no subclass is created. Can you improve the clarity of the comment (and maybe move it above this line, and make it a full sentence)? If you need a suggestion, let me know.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would appreciate your help with a comment.



########################################################################
Expand Down