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.

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

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

Merged
merged 3 commits into from
Feb 23, 2017
Merged

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

merged 3 commits into from
Feb 23, 2017

Conversation

mjpieters
Copy link
Contributor

@mjpieters mjpieters commented Feb 12, 2017

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.

…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
Copy link
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 bpo-28598: Support __rmod__ for RHS subclasses of str in % string formatting operations Feb 12, 2017
@berkerpeksag
Copy link
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
Copy link
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.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

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)?

Python/ceval.c Outdated
PyNumber_Remainder(dividend, divisor);
PyObject *res;
if (PyUnicode_CheckExact(dividend) && !(
PyUnicode_Check(divisor) && !PyUnicode_CheckExact(divisor))) {
Copy link
Member

Choose a reason for hiding this comment

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

Too many negations.

I would expand the expression as PyUnicode_CheckExact(dividend) && (!PyUnicode_Check(divisor) || PyUnicode_CheckExact(divisor)).

Misc/NEWS Outdated
@@ -10,6 +10,8 @@ What's New in Python 3.7.0 alpha 1?
Core and Builtins
-----------------

- Issue #28598: Support __rmod__ for subclasses of str being called before str.__mod__.
Copy link
Member

Choose a reason for hiding this comment

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

Add "Patch by Martijn Pieters."

@codecov
Copy link

codecov bot commented Feb 12, 2017

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

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

@mjpieters
Copy link
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.

colesbury referenced this pull request in colesbury/nogil Oct 6, 2021
jaraco pushed a commit that referenced this pull request Dec 2, 2022
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
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
oraluben pushed a commit to oraluben/cpython that referenced this pull request Jul 11, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants