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

feat(preprocessor): Allow preprocessor to handle binary files #3054

Merged
merged 1 commit into from
Jun 21, 2018

Conversation

kyliau
Copy link
Collaborator

@kyliau kyliau commented Jun 18, 2018

Karma currently detects whether a file is binary, and does not allow binary files to be passed on to preprocessors. This PR removes this restriction, and allows binary files to be handled by preprocessors.

See #3046

@kyliau kyliau requested review from johnjbarton and zzo June 18, 2018 18:58
@@ -125,7 +120,12 @@ function createPreprocessor (config, basePath, injector) {
}

instances[name] = p
preprocessors.push(p)
if (!thisFileIsBinary || p.handleBinaryFiles) {
Copy link
Contributor

@lusarz lusarz Jun 18, 2018

Choose a reason for hiding this comment

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

Do we really need handleBinaryFiles flag and this whole if statement ? Maybe we can just allow to preprocess binary files, and if something fails, preprocessor will throw error.

I guess in more than 90% there will be no binary files that matches pattern and should be preprocessed (Do I miss something ?).

Just thinking out loud, and I'm curious others opinion about that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just for safe backwards compatibility.
Currently if there is a pattern foo/* that matches binary files, it is ignored. Without this flag, the 10% projects that do include binary files in their pattern will break.

Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

Please add a test for this feature.

if (!thisFileIsBinary || p.handleBinaryFiles) {
preprocessors.push(p)
} else {
log.warn('Ignoring preprocessing (%s) %s because it is a binary file.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this message could be clearer as
'Ignoring preprocessing %s because %s has handleBinaryFiles false.'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

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