GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-38208: Simplify string.Template by using __init_subclass__(). by serhiy-storchaka · Pull Request #16256 · python/cpython

Conversation

serhiy-storchaka commented Sep 18, 2019 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

https://bugs.python.org/issue38208

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


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

ce02c9b

serhiy-storchaka added the skip news label

Sep 18, 2019

serhiy-storchaka requested a review from warsaw

September 18, 2019 06:32

serhiy-storchaka requested a review from ericvsmith as a code owner

September 18, 2019 06:32

the-knights-who-say-ni added the CLA signed label

Sep 18, 2019

bedevere-bot added the awaiting core review label

Sep 18, 2019

warsaw requested changes

Sep 18, 2019

View reviewed changes

Comment thread

Lib/dataclasses.py Outdated

		def __repr__(self):
		return f'dataclasses.InitVar[{self.type.__name__}]'

		def __class_getitem__(cls, params):

warsaw Sep 18, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 18, 2019

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/string.py

		braceidpattern = None
		flags = _re.IGNORECASE

		def __init_subclass__(cls):

warsaw Sep 18, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/string.py Outdated

		if 'pattern' in cls.__dict__:
		pattern = cls.pattern
		else:
		pattern = r"""

warsaw Sep 18, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/string.py Outdated

		{(?P<braced>%(bid)s)} \| # delimiter and a braced identifier
		(?P<invalid>) # Other ill-formed delimiter exprs
		)
		""" % {

warsaw Sep 18, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 18, 2019

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 18, 2019

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 18, 2019

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/string.py Outdated

		self.pattern)
		return self.pattern.sub(convert, self.template)

		Template.__init_subclass__() # setup pattern

warsaw Sep 18, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 18, 2019

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting changes and removed awaiting core review labels

Sep 18, 2019

bedevere-bot commented Sep 18, 2019

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka added 2 commits

September 18, 2019 23:10


          Remove unrelated changes.

606a4da


          Address review comments.

68d895b

serhiy-storchaka commented Sep 25, 2019

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka merged commit 919f0bc into python:master

Oct 21, 2019

serhiy-storchaka deleted the refactor-string-template branch

October 21, 2019 06:36

bedevere-bot removed the awaiting changes label

Oct 21, 2019

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request

Dec 5, 2019


          bpo-38208: Simplify string.Template by using __init_subclass__(). (py…

bd881bf

…thonGH-16256)

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request

Jan 31, 2020


          bpo-38208: Simplify string.Template by using __init_subclass__(). (py…

b0640cc

…thonGH-16256)

Budda0ne reviewed

Mar 6, 2021

View reviewed changes

Budda0ne left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Budda0ne reviewed

Mar 6, 2021

View reviewed changes

Budda0ne left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AA-Turner mentioned this pull request

Apr 3, 2025

gh-118761: Optimise import time for string #132037

Merged

Reviewers

warsaw

warsaw requested changes

ericvsmith

Awaiting requested review from ericvsmith

+1 more reviewer

Budda0ne

Budda0ne left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

serhiy-storchaka commented Sep 18, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 18, 2019

Uh oh!

serhiy-storchaka commented Sep 25, 2019

Uh oh!

Budda0ne left a comment

Choose a reason for hiding this comment

Uh oh!

Budda0ne left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

serhiy-storchaka commented Sep 18, 2019 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.