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

Handle invalid byte sequences #39

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

Conversation

jamesstout
Copy link

Re #37, I found a file that triggered the error, it was a ._* macOS resource fork.

My proposed fix is to check the encoding:

unless file_contents.valid_encoding?
    puts "Invalid encoding for #{filename_without_extension}" if options[:verbose]
    file_contents = fix_encoding(file_contents)
end

And if invalid, fix it:

def fix_encoding(file_contents)
    file_contents.encode!('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '')
end

I added my dodgy resource file to the fixtures, it broke the tests [without the new code]. I also added some invalid bytes to unused_class.h to check existing tests pass.

All tests pass. I built the gem and ran against my project with the resource fork, and it worked fine.

Turns out it was am AppleDouble encoded Macintosh file
Added an invalid byte to a test header file, tests with unused_class would fail if fix_encoding is not called.
Signed-off-by: James Stout <[email protected]>
@dangerpr-bot
Copy link

2 Warnings
⚠️ [DEPRECATION] check is deprecated. Please use check! instead.
⚠️ Unless you’re refactoring existing code or improving documentation, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#39](https://github.com/dblock/fui/pull/39): Handle invalid byte sequences - [@jamesstout](https://github.com/jamesstout).

Generated by 🚫 Danger

@dblock
Copy link
Owner

dblock commented Aug 9, 2021

I don't think this is right. Files can have a different encoding and we should support that.

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.

3 participants