File type detection now looks inside the file to determine the type, in addition to using the file extension#3542
Merged
eukreign merged 4 commits intolbryio:masterfrom Jan 31, 2022
Conversation
Member
|
Thanks @eug3nix, review coming shortly! |
eukreign
requested changes
Jan 24, 2022
Member
eukreign
left a comment
eukreign
left a comment
There was a problem hiding this comment.
Thanks for this PR.
Two overall comments:
- this definitely will need tests
- avoid redundant comments. use comments judiciously, only in cases where the code cannot be written clearer without a noticeable performance penalty. comments are not a replacement for writing good, legible, code. use clearer variable names if necessary.
I've also added some inline feedback.
eug3nix
force-pushed
the
gh_3481_file_type_detection
branch
from
9452dac to
babc54a
Compare
removes unnecessary comments
Contributor
Author
|
I've also added a number of tests to cover all cases I could imagine, please take a look @eukreign |
eukreign
approved these changes
Jan 31, 2022
eukreign
changed the title
eukreign
added
area: files
type: improvement
jackrobison
changed the title
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Hi.
This is my attempt at addressing the issue mentioned in bug #3481
I have used a pure-python library for file type detection instead of libmagic binding in order to not introduce extra complexity to the build process and possible issues with other platforms (although libmagic is far superior in its robustness and the amount of supported formats).
Here are some notes on the implementation:
1 - The guess_media_type function was changed so that it tries to open the file from the provided path
2 - If it is able to open the file and detect its type it checks if the extension parsed from the path (extA) is not matching the detected extension (extB)
3 - Then it checks if the extA is a synonym of extB by looking at the synonyms_map, if extB is NOT a synonym, then extA is replaced.
The rest of the code stays the same.
Note0: The extra logic described in step 3 is to handle edge cases when the internals format of the file has an extension which differs from the current file extension but still the current extension is legit. This allows the new code to keep formats such as .cbz (internally a .zip) or cbr (internally a .rar) from being overridden.
Note1: This function may potentially fail with relative file paths in case if the CWD of lbrynet process is not the same as the user's working dir (this is my guess, I have not tested this tbh.)
Please let me know what you think, does this solution is potentially worth of merging, should I add tests or do some other improvements?
Cheers,
Eugene