Skip to content

strings: fix type hints and uncovered bugs - #2555

Merged
williballenthin merged 4 commits into
masterfrom
fix-strings-typing
Jan 16, 2025
Merged

strings: fix type hints and uncovered bugs#2555
williballenthin merged 4 commits into
masterfrom
fix-strings-typing

Conversation

@williballenthin

@williballenthin williballenthin commented Jan 15, 2025

Copy link
Copy Markdown
Collaborator

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

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

mr-tz commented Jan 15, 2025

Copy link
Copy Markdown
Collaborator

Also do this for FLOSS?

@mike-hunhoff mike-hunhoff left a comment

Copy link
Copy Markdown
Collaborator

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 🚀

@fariss fariss left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread capa/features/extractors/strings.py
Comment thread capa/features/extractors/strings.py
Comment thread tests/test_strings.py
@williballenthin

Copy link
Copy Markdown
Collaborator Author

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

@yelhamer yelhamer left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator Author

got the whole crew on this thread!

👋

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