NotificationsYou must be signed in to change notification settings
Fork32k
Star67.3k

bpo-7940: add support for negative end positions to re.finditer and re.findall#14744

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

anilbey wants to merge5 commits intopython:main

base:main

Choose a base branch

fromanilbey:bpo7940

Open

bpo-7940: add support for negative end positions to re.finditer and re.findall#14744

anilbey wants to merge5 commits intopython:mainfromanilbey:bpo7940

+74 −0

Conversation

Copy link

Contributor

anilbey commentedJul 13, 2019•
edited by bedevere-bot
Loading

add support for negative end positions to re.finditer and re.findall

https://bugs.python.org/issue7940

Copy link

the-knights-who-say-ni commentedJul 13, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed thePSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username onbugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please followthe steps outlined in the CPython devguide to rectify this issue.

You cancheck yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

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

Jul 13, 2019

bedevere-bot added the awaiting review label

Jul 13, 2019

isidentical approved these changes

Jul 13, 2019

View reviewed changes

Copy link

Member

isidentical left a comment•
edited
Loading

Choose a reason for hiding this comment

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

LGTM

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

Jul 13, 2019

ezio-melotti requested changes

Jul 13, 2019

View reviewed changes

Modules/_sre.c Outdated

		@@ -427,11 +427,15 @@ state_init(SRE_STATE* state, PatternObject* pattern, PyObject* string,
		}

		/* adjust boundaries */
		if (start < 0)
		start += length;
		if (start < 0)
		start = 0;

Copy link

Member

ezio-melottiJul 13, 2019

Choose a reason for hiding this comment

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

Inre.compile('.').findall("abcd", -10, -3), the firstif will setstart to-6 which is not a valid index, and the second if is necessary because it will set it to0. There are currently no tests for this scenario, so I would add a couple.

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

Jul 13, 2019

Copy link

bedevere-bot commentedJul 13, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

rhettinger self-assigned this

Jul 13, 2019

Copy link

ContributorAuthor

anilbey commentedJul 14, 2019

I have made the requested changes; please review again.

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

Jul 14, 2019

Copy link

bedevere-bot commentedJul 14, 2019

Thanks for making the requested changes!

@ezio-melotti: please review the changes made to this pull request.

the-knights-who-say-ni added CLA signed and removed CLA not signed labels

Jul 15, 2019

rhettinger removed their assignment

Aug 22, 2019

Copy link

Contributor

rhettinger commentedAug 22, 2019

I've looked at this again and am still not inclined to accept it.

If someone else really believes this is a necessary API expansion and wants to move it forward, I won't object.

ezio-melotti approved these changes

Sep 9, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels

Sep 9, 2019

Copy link

Contributor

csabella commentedFeb 4, 2020

@ezio-melotti should this be merged or should there be further discussion?

AlexWaygood added the topic-regex label

Apr 19, 2022

ezio-melotti removed the CLA signed label

Jul 13, 2022

anilbey force-pushed thebpo7940 branch 2 times, most recently froma749312 toa7b0605Compare

July 16, 2022 09:50

Copy link

ContributorAuthor

anilbey commentedJul 16, 2022

Just rebased at another EuroPython sprint after 3 years :)

kumaraditya303 assignedezio-melotti

Jul 16, 2022

Copy link

ContributorAuthor

anilbey commentedJul 16, 2022

Quick summary

the current implementation accepts negative indices but silently truncates them to 0, which I doubt is the expected behaviour
this change allows negative indices to be used the same way they work in other collections
it enables consistency with the regex module
however it introduces a behavioural change

isidentical approved these changes

Jul 16, 2022

View reviewed changes

Copy link

Member

isidentical left a comment

Choose a reason for hiding this comment

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

Re-approving the overall PR, but we need a regex expert on this@ezio-melotti @serhiy-storchaka

Copy link

Member

serhiy-storchaka commentedJul 18, 2022

This is a breaking change. A FutureWarning should be emitted for 2 releases prior to changing the behavior.

Copy link

ContributorAuthor

anilbey commentedJul 18, 2022

This is a breaking change. A FutureWarning should be emitted for 2 releases prior to changing the behavior.

Hi@serhiy-storchaka is that something to be done in the code of this PR? How can we emit a FutureWarning?

iritkatriel requested changes

Nov 25, 2022

View reviewed changes

Copy link

Member

iritkatriel left a comment

Choose a reason for hiding this comment

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

See#14744 (comment)

bedevere-bot added awaiting changes and removed awaiting merge labels

Nov 25, 2022

Copy link

bedevere-bot commentedNov 25, 2022

anilbey force-pushed thebpo7940 branch 3 times, most recently from73147f2 tob3dfb5aCompare

March 15, 2023 10:00

Copy link

ContributorAuthor

anilbey commentedMar 15, 2023

@serhiy-storchaka @iritkatriel I have made the requested changes; please review again. :)

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

Mar 15, 2023

Copy link

bedevere-bot commentedMar 15, 2023

Thanks for making the requested changes!

@iritkatriel,@ezio-melotti,@isidentical: please review the changes made to this pull request.

bedevere-bot requested review fromezio-melotti,iritkatriel andisidentical

March 15, 2023 10:28

Copy link

ContributorAuthor

anilbey commentedMar 15, 2023

@isidentical since your last review I only added the FutureWarnings.

anilbeyand others added4 commits

July 23, 2023 10:46

bpo7940: add support for negative end positions to re.finditer and re…

f47dad7

….findall

bpo7940: added test cases adressing the review

75e7c5f

emit FutureWarning for negative start/end indices

530afdf

test_bug_7940: assure warnings are effective via assertWarns

8bc3fb8

anilbey force-pushed thebpo7940 branch fromb3dfb5a to8bc3fb8Compare

July 23, 2023 08:47

tlynnmannequin mentioned this pull request

Apr 19, 2022

re.finditer and re.findall should support negative end positions#52188

Open

Copy link

ContributorAuthor

anilbey commentedAug 16, 2023•
edited
Loading

Future Warning is emitted in#107105

Merge branch 'main' into bpo7940

46349e5

Copy link

Contributor

dg-pb commentedJun 27, 2024

Maybe it would be worth having global macro for this?
There is one inunicodeobject.c:ADJUST_INDICES, one inbytes_methods.c:ADJUST_INDICES and the same is done here. The same could be used when dealing with sequence indices too.

Copy link

ContributorAuthor

anilbey commentedJun 27, 2024

Hi@dg-pb,

Thank you for your reply. This PR has been open for about 5 years now. Are you a core developer, and would you be interested in seeing this change merged?

Copy link

Contributor

dg-pb commentedJun 27, 2024

Hi@dg-pb,
Thank you for your reply. This PR has been open for about 5 years now. Are you a core developer, and would you be interested in seeing this change merged?

I am not. Just decided to go through old PRs and decided to offer my thoughts for this one. Hopefully someone familiar withre can resolve whether to close or implement this.