FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-38208: Simplify string.Template by using __init_subclass__(). by serhiy-storchaka · Pull Request #16256 · python/cpython · GitHub

/ cpython Public

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

Merged
serhiy-storchaka merged 3 commits into
python:masterfrom
serhiy-storchaka:refactor-string-template
Oct 21, 2019
Merged

bpo-38208: Simplify string.Template by using __init_subclass__().#16256
serhiy-storchaka merged 3 commits into
python:masterfrom
serhiy-storchaka:refactor-string-template

Conversation

serhiy-storchaka commented Sep 18, 2019
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Comment thread Lib/dataclasses.py Outdated
def __repr__(self):
return f'dataclasses.InitVar[{self.type.__name__}]'

def __class_getitem__(cls, params):

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh, yes. They are from other issue.

Comment thread Lib/string.py
braceidpattern = None
flags = _re.IGNORECASE

def __init_subclass__(cls):

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Lib/string.py Outdated
if 'pattern' in cls.__dict__:
pattern = cls.pattern
else:
pattern = r"""

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Lib/string.py Outdated
{(?P<braced>%(bid)s)} | # delimiter and a braced identifier
(?P<invalid>) # Other ill-formed delimiter exprs
)
""" % {

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

Comment thread Lib/string.py Outdated
self.pattern)
return self.pattern.sub(convert, self.template)

Template.__init_subclass__() # setup pattern

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would appreciate your help with a comment.

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Copy link
Copy Markdown
Member Author

@warsaw, what is your thought about using an f-string?

serhiy-storchaka merged commit 919f0bc into python:master Oct 21, 2019
serhiy-storchaka deleted the refactor-string-template branch October 21, 2019 06:36

Budda0ne left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok

Budda0ne left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL