Skip to content

strings: fix type hints and uncovered bugs #2555

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

Merged
merged 4 commits into from
Jan 16, 2025
Merged

Conversation

williballenthin
Copy link
Collaborator

@williballenthin williballenthin commented Jan 15, 2025

When adding type hints (at the suggestion of my editor), I found that some of the strings.py helpers probably didn't ever work on python3. So I fixed those up and added some tests.

TODO

  • changelog
  • tests
  • No documentation update needed

@williballenthin williballenthin added the bug Something isn't working label Jan 15, 2025
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased) section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed

@github-actions github-actions bot dismissed their stale review January 15, 2025 12:17

CHANGELOG updated or no update needed, thanks! 😄

@williballenthin williballenthin force-pushed the fix-strings-typing branch 2 times, most recently from e5b59d7 to 4678af7 Compare January 15, 2025 12:46
changelog

add strings tests

strings: fix buf_filled_with

fix strings tests

refactor: optimize and document buf_filled_with function in strings.py

docs: add docstring to buf_filled_with function

doc

strings: add typing
@williballenthin williballenthin marked this pull request as ready for review January 15, 2025 12:57
@mr-tz
Copy link
Collaborator

mr-tz commented Jan 15, 2025

Also do this for FLOSS?

Copy link
Collaborator

@mike-hunhoff mike-hunhoff left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the tests! LGTM 🚀

Copy link
Collaborator

@fariss fariss left a comment

Choose a reason for hiding this comment

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

Good improvements, thanks. I have left some comments for your review.

@williballenthin
Copy link
Collaborator Author

these are all great ideas @fariss thank you! i'll add those in the morning and merge. thanks all

Copy link
Collaborator

@yelhamer yelhamer left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

Comment on lines 20 to 21
ASCII_RE_4 = re.compile(b"([%s]{%d,})" % (ASCII_BYTE, 4))
UNICODE_RE_4 = re.compile(b"((?:[%s]\x00){%d,})" % (ASCII_BYTE, 4))
Copy link
Collaborator

Choose a reason for hiding this comment

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

imo 4 here is not very descriptive, I'd suggest declaring it a constant:

MIN_STRING_LENGTH = 4 

ASCII_RE_4 = re.compile(b"([%s]{%d,})" % (ASCII_BYTE, MIN_STRING_LENGTH))
UNICODE_RE_4 = re.compile(b"((?:[%s]\x00){%d,})" % (ASCII_BYTE, MIN_STRING_LENGTH))

@williballenthin
Copy link
Collaborator Author

got the whole crew on this thread!

👋

@williballenthin williballenthin merged commit 72fe291 into master Jan 16, 2025
23 checks passed
@williballenthin williballenthin deleted the fix-strings-typing branch January 16, 2025 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants