irc.utils, test: also remove null (\x00) in safe() #2620
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In `test_irc_utils` suite, rewrote `test_safe` and `test_safe_bytes` to generate a bunch more permutations of invalid characters automatically rather than writing them all out. The horse is definitely beaten *far* past death at this point... but the tests sure are thorough(bred). Also renamed `test_safe_null` -> `test_safe_none` to avoid confusion; `test_safe` and `test_safe_bytes` are the cases that check behavior with null characters/bytes. Python's docs technically do refer to `None` as the "null object" once or twice, but most of the docs (and most of us devs) simply call it `None`.
dgw
added
the
Bugfix
Exirel
requested changes
Sep 15, 2024
Using `pytest.mark.parametrize()` instead. Two test cases become 12. Note: Yes, it is possible to have pytest treat each tuple generated by `itertools.permutations()` as a single argument. Doing so yields useless generated test-case names like `test_safe[seq0]`, though. Using three separate test parameters of simple types (`str` or `bytes`) yields more informative names like `test_safe[\n-\r-\x00]`.
Exirel
approved these changes
Oct 11, 2024
SnoopJ
approved these changes
Oct 12, 2024
|
Hey, this one could easily go into Sopel 8.0.1 to me. Like a hotfix of something that should be done but isn't. |
Yeah, it is, isn't it? I will tweak the |
Link to RFC explaining why CR, LF, and NUL are prohibited, and annotate the version in which NUL stripping was added.
|
Netlify still does branch previews of our docs (for now… they are still stuck on Python 3.8 with no apparent plans to upgrade) but including a preview screenshot of the updated docs for this function anyway didn't seem like a bad idea.
Note I also fixed the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2566.
In
test_irc_utilssuite, rewrotetest_safeandtest_safe_bytesto generate a bunch more permutations of invalid characters automatically rather than writing them all out. The horse is definitely beaten far past death at this point... but the tests sure are thorough(bred).Also renamed
test_safe_null->test_safe_noneto avoid confusion;test_safeandtest_safe_bytesare the cases that check behavior with null characters/bytes. Python's docs technically do refer toNoneas the "null object" once or twice, but most of the docs (and most of us devs) simply call itNone.Checklist
make qa(runsmake lintandmake test)make lintmyself, and tried out the test runner integration in VS Code / Gitpod. Very slick; made iterating my updated tests much easier without always tabbing back to the terminal.Notes
This could go into 8.0.1, but I don't really want to mess around with hotfixes unless there's a major bug. I'm open to suggestions for improving the maintenance-branch workflow; since
masteris de facto for 8.1.0 dev now, the options are to keep the8.0.xbranch unprotected so I can push cherry-picks to it directly, or make @sopel-irc/rockstars review the same PR twice (once tomaster, once to8.0.x) for full CI coverage. I don't love either of those.