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

find: support escaping backslash, fix double-bolding, and add basic tests #2589

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

dgw
Copy link
Member

@dgw dgw commented Jan 27, 2024

Description

Allowing \| or \/ is all well and good, but if one can't escape the escape character, things get very confusing! So let's make it possible to escape \ itself as \\.

With the test scaffolding now written (OK, much of it stolen from the preexisting test/builtins/test_builtins_isup.py), it'll be easier to add more cases (edge or otherwise) as we think of them.

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 lint and make test)
  • I have tested the functionality of the things this change touches

Notes

Early 8.0.x bugfix candidate, unless someone else feels strongly about including it in 8.0.0. Feels late in the game to add One More Thing into that milestone…

@dgw dgw added Tests Bugfix Generally, PRs that reference (and fix) one or more issue(s) labels Jan 27, 2024
@dgw dgw added this to the 8.0.1 milestone Jan 27, 2024
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.

First, I love that you are using the testing tools for that. I'm happy to see it works quite well. I've suggested a couple of quick changes, that makes the test less reliable on hard-coded value (the user's nick).

I think it would be valuable to add two other tests:

  1. one where there are 2 users, and one say something, the other use the replace feature
  2. one where a user replace the replaced line

And with that, I believe we would have a pretty good set of tests.

test/builtins/test_builtins_find.py Outdated Show resolved Hide resolved
test/builtins/test_builtins_find.py Outdated Show resolved Hide resolved
Copy link
Member Author

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Points well taken about additional tests. I added a few more test cases, and also fixed another bug: the double-\x02 wrapping of replacements inside previous replacements.

test/builtins/test_builtins_find.py Outdated Show resolved Hide resolved
test/builtins/test_builtins_find.py Outdated Show resolved Hide resolved
@dgw dgw changed the title find: double-backslash support + basic tests find: support escaping backslash, fix double-bolding, and add basic tests Sep 2, 2024
@dgw dgw requested a review from Exirel September 2, 2024 04:52
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.

Love to see it!

dgw and others added 4 commits October 13, 2024 11:25
Co-authored-by: Florian Strzelecki <florian.strzelecki@gmail.com>
Check that correcting someone else's line works as expected, and check
that replacing an already-replaced line works as expected.

Also check that case-insensitive (i) and global (g) flags work as
expected, both separately and together.

Note: There is a glitch in formatting when the new replacement occurs
inside the previous one, since the bolding used to indicate what changed
is kept in the stored line. We might wish to fix that at some point, but
the new test correctly represents the real behavior.
Perform the replacement with an unformatted version of the substitute
string, then create a display version with bolding added only after the
matching line (if any) is found.

Updated the corresponding test case. Also renamed some local vars in the
`find` plugin file to make a bit more sense (but not too dramatic).
@dgw
Copy link
Member Author

dgw commented Oct 13, 2024

Took care of a squashme: commit. No changes to the patch itself; the commit history is just cleaner.

Side benefit: CI re-runs after a force-push, and it verified the new tests on Python 3.13 for me. 😁

@dgw dgw merged commit 74d60f4 into master Oct 14, 2024
16 checks passed
@dgw dgw deleted the find-escaped-escapechar branch October 14, 2024 15:44
dgw added a commit that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s) Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants