Skip to content

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

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

Merged
serhiy-storchaka 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
bpo-38208: Simplify string.Template by using __init_subclass__().
  • Loading branch information
@serhiy-storchaka
serhiy-storchaka committed Sep 18, 2019
commit ce02c9ba0655f43923ce5e9369f1bf88f9bb30e8
9 changes: 4 additions & 5 deletions Lib/dataclasses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
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):

@warsaw warsaw Sep 18, 2019

Copy link
Copy Markdown
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.

@serhiy-storchaka serhiy-storchaka Sep 18, 2019

Copy link
Copy Markdown
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
Original file line number Diff line number Diff line change
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):

@warsaw warsaw Sep 18, 2019

Copy link
Copy Markdown
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"""

@warsaw warsaw Sep 18, 2019

Copy link
Copy Markdown
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
)
""" % {

@warsaw warsaw Sep 18, 2019

Copy link
Copy Markdown
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.

@serhiy-storchaka serhiy-storchaka Sep 18, 2019

Copy link
Copy Markdown
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.

@serhiy-storchaka serhiy-storchaka Sep 18, 2019

Copy link
Copy Markdown
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.

@serhiy-storchaka serhiy-storchaka Sep 18, 2019

Copy link
Copy Markdown
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

@warsaw warsaw Sep 18, 2019

Copy link
Copy Markdown
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.

@serhiy-storchaka serhiy-storchaka Sep 18, 2019

Copy link
Copy Markdown
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