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

Add option to include/exclude binary files #54

Merged
merged 1 commit into from
Oct 5, 2016

Conversation

jpmv27
Copy link
Contributor

@jpmv27 jpmv27 commented Oct 5, 2016

This is how I implemented #48, not quite as you suggested, but with a Boolean parameter that indicates whether the tool includes or excludes binary files by default.
Please take a look and I will rework as required.
Once the code is done to your satisfaction, I'll update the docs.

Copy link
Owner

@dkprice dkprice left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for implementing! The only piece that will need to be added is to have an dictionary entry for turning binary off. 'sift', for example, has --binary-skip. sift is experimental at this point though so I don't see any reason to have that hold this up.

@dkprice dkprice merged commit c60c4ce into dkprice:master Oct 5, 2016
@jpmv27
Copy link
Contributor Author

jpmv27 commented Oct 5, 2016

Thanks. But I'm not sure I understand your comment about sift.

Wouldn't the configuration for sift be similar to that for grep?

'opt_str_binary_switch': '--binary-skip'
'opt_bool_binary_excluded_by_default': '0'

Perhaps I misunderstand something
MV

@jpmv27
Copy link
Contributor Author

jpmv27 commented Oct 5, 2016

Also, do you want a separate PR for adding the new option to the docs, or just update this one?

@dkprice
Copy link
Owner

dkprice commented Oct 7, 2016

My mistake, you're correct. This works nicely. A separate PR for the docs would be good.

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.

2 participants