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.

Sign up for GitHub

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

Jump to bottom

PR: Fix issue where replace all did not respect whole words filter #20497

Merged
merged 7 commits into from
Feb 11, 2023
Merged

PR: Fix issue where replace all did not respect whole words filter #20497

merged 7 commits into from
Feb 11, 2023

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented Feb 7, 2023

Description of Changes

  • Update replace pattern to respect whole words filter
  • Added regression test

Issue(s) Resolved

Fixes #20403

@mrclary mrclary self-assigned this Feb 7, 2023
@ccordoba12 ccordoba12 changed the base branch from 5.x to master February 9, 2023 15:43
@ccordoba12 ccordoba12 added this to the v6.0alpha1 milestone Feb 9, 2023
@ccordoba12
Copy link
Member

ccordoba12 commented Feb 9, 2023 •
edited
Loading

@mrclary, I changed the base branch of your PR to master because we agreed that from now on only critical things or important bug fixes should be addressed for Spyder 5. And I don't think this is really critical.

@mrclary
Copy link
Contributor Author

mrclary commented Feb 9, 2023

@mrclary, I changed the base branch of your PR to master because we agreed that from now on only critical things or important bug fixes should be addressed for Spyder 5. And I don't think this is really critical.

I'll rebase my branch off master as well.

ccordoba12
ccordoba12 reviewed Feb 10, 2023
View reviewed changes
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @mrclary for your work on this!

spyder/widgets/findreplace.py Show resolved Hide resolved
spyder/widgets/findreplace.py Outdated Show resolved Hide resolved
spyder/widgets/findreplace.py Show resolved Hide resolved
spyder/widgets/tests/test_findreplace.py Outdated Show resolved Hide resolved
spyder/widgets/tests/test_findreplace.py Outdated Show resolved Hide resolved
ccordoba12
ccordoba12 reviewed Feb 10, 2023
View reviewed changes
Copy link
Member

@ccordoba12 ccordoba12 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.

One last stylistic change that I missed before, then this should be ready for me.

spyder/widgets/findreplace.py Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Member

@impact27, do you have something else to say about this one?

@impact27
Copy link
Contributor

impact27 commented Feb 11, 2023 •
edited
Loading

Looks good to me, I would just add a comment explaining replace_text = replace_text.replace('\\', r'\\') (re.sub processes backslashes so it must be escaped)

ccordoba12
ccordoba12 approved these changes Feb 11, 2023
View reviewed changes
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @mrclary!

@ccordoba12 ccordoba12 merged commit 450e50d into spyder-ide:master Feb 11, 2023
@mrclary mrclary deleted the issue-20403-replace-all branch February 11, 2023 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@impact27 impact27 impact27 left review comments

@ccordoba12 ccordoba12 ccordoba12 approved these changes

Assignees

@mrclary mrclary

Labels
component:Find and Replace type:Bug
Projects
None yet
Milestone
v6.0alpha1
Development

Successfully merging this pull request may close these issues.

Find and replace "Replace all occurences" ignores "Only search for whole words"
3 participants
@mrclary @ccordoba12 @impact27