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

find: format replacement text instead of "meant" #2014

Merged
merged 2 commits into from
Feb 13, 2021
Merged

find: format replacement text instead of "meant" #2014

merged 2 commits into from
Feb 13, 2021

Conversation

cottongin
Copy link
Contributor

Description

This simply applies the bold formatting to whatever the new replacement text is rather than the "meant" text.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

cottongin and others added 2 commits January 9, 2021 19:53
@cottongin @dgw
find: format replacement text instead of "meant"
cd273e4 
This simply applies the bold formatting to whatever the new replacement
text is rather than the "meant" text.
@dgw
find: eliminate double string comparison
c37ad2e 
I can think of no reason to compare `line` and `new_phrase` twice rather
than just using a temporary variable inside the loop and assigning the
replacement result to `new_phrase` only if it changed.
@dgw dgw added this to the 7.1.0 milestone Jan 10, 2021
@dgw dgw added the Tweak label Jan 10, 2021
@dgw
Copy link
Member

dgw commented Jan 10, 2021 •
edited
Loading

I was almost ready to approve, but I saw a bit of code between the existing changes that bugged me & rewrote it. That disqualifies me from reviewing this PR objectively (because part of it was my own work). 😅

@dgw dgw requested a review from a team January 10, 2021 01:57
Exirel
Exirel approved these changes Jan 10, 2021
View reviewed changes
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

Fine by me. I'm not bothered by any of the version about new_phrase/replaced stuff.

@dgw dgw merged commit a56edd5 into sopel-irc:master Feb 13, 2021
@dgw
Copy link
Member

dgw commented Feb 13, 2021

@cottongin merged manually to spare you the pain of rebasing

@dgw dgw mentioned this pull request May 15, 2021
Merged
4 tasks
@dgw dgw mentioned this pull request Sep 15, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Exirel Exirel Exirel approved these changes

Assignees
No one assigned
Labels
Tweak
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cottongin @dgw @Exirel