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

Extend default skip magic rather than overwrite it. #693

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

qkaiser
Copy link
Contributor

@qkaiser qkaiser commented Dec 24, 2023

unblob has a decent default skip magic list that gets overwritten if a user provides its own, which means unblob users need to redefine all of unblob's default skip magic through the CLI whenever they provide their own.

Changed the logic so that user provided skip magic values are simply appended to unblob's default list.

This is an opinonated change, but we think unblob default skip magic is well thought out and the inability to fully overwrite it should not be a problem for the vast majority of our user base.

Related to #647

@qkaiser qkaiser added the enhancement New feature or request label Dec 24, 2023
@qkaiser qkaiser self-assigned this Dec 24, 2023
@qkaiser qkaiser linked an issue Dec 24, 2023 that may be closed by this pull request
Copy link
Contributor

@e3krisztian e3krisztian left a comment

Choose a reason for hiding this comment

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

The intention behind replacing the skip list is that by doing so we can force extraction one of the file types on this list, even if that would be skipped by default.

Changing skip magic specification to extend only would prevent clearing this magic-skip list, so unblob would be unable to extract file types that are on its magic-skip list, even if technically it has a handler for it.

Maybe a better extension would be to add a new --extend-default-skip-magic bool option, which would add the DEFAULT_SKIP_MAGIC to the user supplied list.

This solution would let the users extend the magic-skip list, while not changing the current behavior.


Alternatively we could have a --clear-skip-magic bool option, which would be needed to get the current behavior (on main), replacing the magic skip list. Without --clear-skip-magic the DEFAULT_SKIP_MAGIC would be added to the user supplied (--skip-magic) magics.

I would prefer this alternative, as I think adding a new magic is more common/natural than replacing the whole list, even if it is an unblob behavior change.

I think this --clear-skip-magic alternative would be easier to understand and use, than the --extend-default-skip-magic variant.

unblob/processing.py Outdated Show resolved Hide resolved
unblob/processing.py Outdated Show resolved Hide resolved
@qkaiser qkaiser force-pushed the extend-skip-magic branch 2 times, most recently from 524846c to 62f1398 Compare January 3, 2024 14:55
@qkaiser qkaiser requested a review from e3krisztian January 3, 2024 14:56
unblob/cli.py Outdated Show resolved Hide resolved
@qkaiser qkaiser force-pushed the extend-skip-magic branch 2 times, most recently from ddb81e5 to 4853897 Compare January 3, 2024 16:24
@qkaiser qkaiser requested a review from e3krisztian January 4, 2024 08:39
…ite it.

unblob has a decent default skip magic list that gets overwritten if a
user provides its own, which means unblob users need to redefine all of
unblob's default skip magic through the CLI whenever they provide their
own.

Changed the logic so that user provided skip magic values are simply
appended to unblob's default list unless the user explicitly provides the
"--clear-skip-magics" flag.

Co-authored-by: Krisztián Fekete <[email protected]>
Copy link
Contributor

@e3krisztian e3krisztian left a comment

Choose a reason for hiding this comment

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

This is a backward incompatible behavior change for the --skip-magic option.

The original way was somewhat surprising and harder to use (#647).

@qkaiser qkaiser merged commit 7d3b935 into main Jan 4, 2024
17 checks passed
@qkaiser qkaiser deleted the extend-skip-magic branch January 4, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend --skip-magic option for better matching
2 participants