◐ Shell
reader mode source ↗
Skip to content

bpo-28598: Support __rmod__ for RHS subclasses of str in % string formatting operations#51

Merged
serhiy-storchaka merged 3 commits into
python:masterfrom
mjpieters:issue28598_str_rmod_fix
Feb 23, 2017
Merged

bpo-28598: Support __rmod__ for RHS subclasses of str in % string formatting operations#51
serhiy-storchaka merged 3 commits into
python:masterfrom
mjpieters:issue28598_str_rmod_fix

Conversation

@mjpieters

@mjpieters mjpieters commented Feb 12, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

When you use '%s' % SubClassOfStr(), where SubClassOfStr.__rmod__ exists, the reverse operation is ignored as normally such string formatting operations use the PyUnicode_Format() fast path. This patch tests for subclasses of str first and picks the slow path in that case.

Fixes bpo-28598.

Support __rmod__ for RHS subclasses of str in % string formatting ope… …
1ef5f55 
…rations.

When you use `'%s' % SubClassOfStr()`, where `SubClassOfStr.__rmod__` exists, the reverse operation is ignored as normally such string formatting operations use the `PyUnicode_Format()` fast path. This patch tests for subclasses of `str` first and picks the slow path in that case.

Fixes issue28598.
@mjpieters

mjpieters commented Feb 12, 2017

Copy link
Copy Markdown
Contributor Author

Open question: Should this patch be backported to 3.6 and 3.5? It's a bug fix, not a new feature.

@berkerpeksag berkerpeksag changed the title Support __rmod__ for RHS subclasses of str in % string formatting operations Feb 12, 2017
@berkerpeksag

berkerpeksag commented Feb 12, 2017

Copy link
Copy Markdown
Member

Should this patch be backported to 3.6 and 3.5?

Yes, it should be backported if it's a bug (I haven't investigated myself yet.)

I'm not sure if issues are linked automatically yet.

We don't use issueNNNN anymore. The correct way to link PRs to bugs.p.o is bpo-NNNN.

@mjpieters

mjpieters commented Feb 12, 2017

Copy link
Copy Markdown
Contributor Author

We don't use issueNNNN anymore. The correct way to link PRs to bugs.p.o is bpo-NNNN.

Ta, also for the title change! Updated the 'fixes' line too.

serhiy-storchaka
serhiy-storchaka reviewed Feb 12, 2017
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

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.

Hide comment

In general LGTM except few minor details.

And I'm not sure about the style of commit messages now. Should it include issue number at the first line? Should it contain "Patch by YYY." (now the author is specified in metadata)?

@codecov

codecov Bot commented Feb 12, 2017

Copy link
Copy Markdown

Codecov Report

Merging #51 into master will decrease coverage by -0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   82.37%   82.37%   -0.01%     
==========================================
  Files        1427     1427              
  Lines      350948   350953       +5     
==========================================
- Hits       289099   289097       -2     
- Misses      61849    61856       +7

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af88e7e...7b19b02. Read the comment docs.

@brettcannon

brettcannon commented Feb 13, 2017

Copy link
Copy Markdown
Member

Should this really be backported? It seems like a change in semantics more than a bugfix.

@mjpieters

mjpieters commented Feb 14, 2017

Copy link
Copy Markdown
Contributor Author

@brettcannon This is definitely a bug, not a change in semantics. From the object.__rmod__ documentation:

Note: If the right operand’s type is a subclass of the left operand’s type and that subclass provides the reflected method for the operation, this method will be called before the left operand’s non-reflected method. This behavior allows subclasses to override their ancestors’ operations.

str % subclass_of_str doesn't follow this behavior, and str is the only type that doesn't (everything else goes through the PyNumber_Remainder() slow path). This patch fixes that bug.

This was referenced Feb 14, 2017
Closed
Closed
Merged
Merged
@serhiy-storchaka serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Feb 18, 2017
@methane methane mentioned this pull request Feb 21, 2017
Merged
@serhiy-storchaka serhiy-storchaka merged commit d7e6433 into python:master Feb 23, 2017
@zhangyangyu zhangyangyu mentioned this pull request Mar 1, 2017
Merged
@mjpieters mjpieters mentioned this pull request Mar 1, 2017
Closed
@mjpieters mjpieters deleted the issue28598_str_rmod_fix branch March 1, 2017 16:55
140 hidden items Load more…
colesbury referenced this pull request in colesbury/nogil Oct 6, 2021
@colesbury
Add a lock around dtoa mult5 code …
ec46739 
See #51
@tendermonster tendermonster mentioned this pull request Oct 19, 2022
Closed
jaraco pushed a commit that referenced this pull request Dec 2, 2022
@Mariatta
Merge the PR with one core dev approval and CI passed. (GH-51) …
69f93c6 
Allow miss-islington to merge the PR if all of these conditions met:
- All CI passed
- At least one core dev approved
- PR is made by miss-islington
jaraco pushed a commit to jaraco/cpython that referenced this pull request Feb 17, 2023
@warsaw
Merge branch 'issue51' into 'master' …
bd5afac 
Back port latest changes to importlib.resources

Closes python#51

See merge request python-devs/importlib_resources!55
mgmacias95 pushed a commit to mgmacias95/cpython that referenced this pull request Mar 24, 2023
@pablogsal
Merge pull request python#51 from isidentical/more-simplifications
b41a3b8
oraluben pushed a commit to oraluben/cpython that referenced this pull request Jul 11, 2023
@Fidget-Spinner
feat: bigger CACHE entries for everything, CACHE for JUMP_BACKWARD (p… …
7210772 
…ython#51)

* feat: store bb_test flag in frame to reload during resume

* feat: bigger CACHE entries for everything, CACHE for JUMP_BACKWARD

* nit: increase overallocate factor even more

* feat: remove bb_flag/bb_test and use only the frame's
@gvanrossum gvanrossum mentioned this pull request Aug 22, 2023
Closed
@LinanV LinanV mentioned this pull request Oct 23, 2023
Closed
@kcatss kcatss mentioned this pull request Feb 20, 2024
Closed
@ngoldbaum ngoldbaum mentioned this pull request Sep 23, 2024
Closed
This was referenced Feb 11, 2025
Closed
Closed
@sergey-miryanov sergey-miryanov mentioned this pull request Dec 18, 2025
Closed
DinoV added a commit to DinoV/cpython that referenced this pull request Feb 11, 2026
@DinoV
Merge pull request python#51 from DinoV/fix_merge …
80b3925 
Fix bad merge
@ngoldbaum ngoldbaum mentioned this pull request Mar 6, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees

No one assigned

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mjpieters @berkerpeksag @brettcannon @serhiy-storchaka @zhangyangyu @Mariatta @the-knights-who-say-ni