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

Replace binascii and struct with native Python methods #2582

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

v1bh475u
Copy link
Contributor

@v1bh475u v1bh475u commented Jan 30, 2025

Checklist

closes #2327

  • No CHANGELOG update needed
  • No new tests needed
  • No documentation update needed

Copy link
Collaborator

@williballenthin williballenthin left a comment

Choose a reason for hiding this comment

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

some of these changes i like, such as the hex conversions. those are much cleaner.

but the migration from using struct doesn't seem as clear cut. what benefits do the new code constructs bring? i find the lines much noisier (lots of repeated kwargs) and therefore hard to read. given that struct is a built in Python module, i don't yet understand the desire to remove it here. can you explains a bit @v1bh475u ?

capa/features/extractors/elf.py Outdated Show resolved Hide resolved
@v1bh475u
Copy link
Contributor Author

v1bh475u commented Jan 31, 2025

i find this region to be very noisy and hard to read.

I also felt that way but thought the issue wished for removing dependency of struct. Personally, I feel struct code looks much cleaner wherever struct.unpack_from is used. Shall I revert the commits for struct and make another one which does not remove those lines?

@williballenthin
Copy link
Collaborator

that would be great! then we can continue to discuss struct vs int separately.

@v1bh475u
Copy link
Contributor Author

Done with changes.

Copy link
Collaborator

@williballenthin williballenthin left a comment

Choose a reason for hiding this comment

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

these look great. please give yourself some credit in the changelog!

@v1bh475u
Copy link
Contributor Author

under what header should I add?

@williballenthin
Copy link
Collaborator

bug fixes

@fariss
Copy link
Collaborator

fariss commented Jan 31, 2025

I agree with the fact that the repeated kwargs make the code not easy to read.

some of these changes i like, such as the hex conversions. those are much cleaner.

That's exactly my initial thought when suggesting to use int. Generally speaking, struct is usually used to pack a lot of data together with longer format strings (See discussion here around the use of struct vs int https://stackoverflow.com/a/56799074)

For consistency, let's keep using struct for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consider replacing binascii and struct with native Python methods
3 participants