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(web-server): Use isbinaryfile for binary file detection #1816

Merged
merged 1 commit into from
Jan 22, 2016
Merged

feat(web-server): Use isbinaryfile for binary file detection #1816

merged 1 commit into from
Jan 22, 2016

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jan 21, 2016

Delete lib/binary-extensions.json and change the preprocessor to use the isbinaryfile module. This is a very simple change but there are some whitespace only changes due to the isBinaryFile function being asynchronous and therefore adding an extra level of indentation, append ?w=1 to the PR url to have the diff ignore whitespace.

Closes #1070

@GitCop
Copy link

GitCop commented Jan 21, 2016

Thank you for your contribution. We detected an issue with your commit message though and would ask you kindly to fix it.

  • Commit: 12d1085
    • Your commit message body contains a line that is longer than 80 characters

Guidelines are available at http://karma-runner.github.io/0.13/dev/git-commit-msg.html and please do not hesitate to ask if you have any questions.


This message was auto-generated by https://gitcop.com

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot
Copy link

CLAs look good, thanks!

@@ -249,7 +249,8 @@
"Jon Bretman <jon.bretman@gmail.com>",
"Julian Connor <julian.connor@venmo.com>",
"Jurko Gospodnetić <jurko.gospodnetic@pke.hr>",
"Karl Lindmark <karl.lindmark@ninetwozero.com>"
"Karl Lindmark <karl.lindmark@ninetwozero.com>",
"Matthew Amato <matt.amato@gmail.com>"
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this, we generate this list automatically on release via the git log

@dignifiedquire
Copy link
Member

Thanks, left some comments inline

@mramato
Copy link
Contributor Author

mramato commented Jan 21, 2016

Thanks, left some comments inline

Thanks, should I squash this PR back into a single commit after making changes?

@GitCop
Copy link

GitCop commented Jan 21, 2016

Thank you for your contribution. We detected an issue with your commit message though and would ask you kindly to fix it.

  • Commit: c9b6479
    • Commits must be in the following format: %{type}(?%?{scope})?: %{description}
    • Your subject line is longer than 70 characters

Guidelines are available at http://karma-runner.github.io/0.13/dev/git-commit-msg.html and please do not hesitate to ask if you have any questions.


This message was auto-generated by https://gitcop.com

var fs = require('graceful-fs')
var crypto = require('crypto')
var mm = require('minimatch')
var extensions = require('./binary-extensions.json').extensions
var isBinaryFile = require('isBinaryFile')
Copy link
Member

Choose a reason for hiding this comment

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

looks like this needs to be downcased to require('isbinaryfile') (see travis failures)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, thanks. Stupid Windows case insensitivity.

@dignifiedquire
Copy link
Member

Thanks, should I squash this PR back into a single commit after making changes?

Yes for things like this, there should be a single commit when I merge it.

@GitCop
Copy link

GitCop commented Jan 21, 2016

Thank you for your contribution. We detected an issue with your commit message though and would ask you kindly to fix it.

  • Commit: c9b6479
    • Commits must be in the following format: %{type}(?%?{scope})?: %{description}
    • Your subject line is longer than 70 characters
  • Commit: 22699db
    • Commits must be in the following format: %{type}(?%?{scope})?: %{description}

Guidelines are available at http://karma-runner.github.io/0.13/dev/git-commit-msg.html and please do not hesitate to ask if you have any questions.


This message was auto-generated by https://gitcop.com

@dignifiedquire
Copy link
Member

The error comes from you using new Buffer() without any content. If you want to create an empty buffer just do sth like new Buffer(20).fill(0)

@mramato
Copy link
Contributor Author

mramato commented Jan 21, 2016

The error comes from you using new Buffer() without any content. If you want to create an empty buffer just do sth like new Buffer(20).fill(0)

Thanks, I realized that already now I'm trying to figure out why tests are still failing. I think there is an issue with the isbinaryfile module and the mocking that's going on in the tests. I'll let you know once I figure it out.

@mramato
Copy link
Contributor Author

mramato commented Jan 21, 2016

On an unrelated note, the validate-commit-msg step crashes on my system whenever I try to commit. My only option is to comment out the step in package.json temporarily (which is my the bot yelled at me a few times). Here's the callstack:

C:\Git\mramato\karma\node_modules\validate-commit-msg\index.js:103
      return buffer.toString().split('\n').shift();
                   ^

TypeError: Cannot read property 'toString' of undefined
    at firstLineFromBuffer (C:\Git\mramato\karma\node_modules\validate-commit-msg\index.js:103:20)
    at ReadFileContext.callback (C:\Git\mramato\karma\node_modules\validate-commit-msg\index.js:92:15)
    at FSReqWrap.readFileAfterOpen [as oncomplete] (fs.js:303:13)

Should I file a new issue for this? I'm on Windows 7, Node 4.2.x LTS

@dignifiedquire
Copy link
Member

please file an issue for this on the validate-commit-msg repo

@mramato
Copy link
Contributor Author

mramato commented Jan 21, 2016

All of my changes are in and have been squashed into a single commit.

I just noticed that two tests are failing on Node 0.10 tests (but tests pass on all other versions), but it looks like Node 0.10 fails in karma master too. I'm not sure why my branch is failing other than possible incompatibility between isbinaryfile module and 0.10. If anyone has any thoughts, I'd appreciate it. Otherwise, I can try 0.10 under nvm when I'm home on my Linux machine. I assume 0.10 compatibility is still required?

@dignifiedquire
Copy link
Member

Yes we need 0.10 compat, but those tests are just time sensitive and flaky on travis for that reason. I've retriggered it and it should be going green soon :)

@dignifiedquire
Copy link
Member

Hmm looks like this is a real failure on 0.10 now :( if you can take a look later today I'd appreciate it. Otherwise I can check it out tomorrow

@mramato
Copy link
Contributor Author

mramato commented Jan 22, 2016

So I ran the tests over 40 times locally under v0.10.41 on Linux Mint 17.3 and everything passes. I have no idea why they failed on travis.

@mramato
Copy link
Contributor Author

mramato commented Jan 22, 2016

Nevermind, I'm a moron and forgot to check out my branch after the initial clone on my linux partition. The tests fail every time.

Delete `lib/binary-extensions.json` and change the preprocessor to use the
`isbinaryfile` module instead.

Factor nextPreprocessor function creation into a factory function for
code clarity.

Update preprocessor specs to use binary data when mocking binary files.

Closes #1070
@mramato
Copy link
Contributor Author

mramato commented Jan 22, 2016

Okay, I've addressed all problems and squashed my latest changes into a single commit. This is ready for final review (and hopefully merger). Thanks for the help.

@dignifiedquire
Copy link
Member

Thanks :octocat: this looks good to me :)

dignifiedquire added a commit that referenced this pull request Jan 22, 2016
feat(web-server): Use isbinaryfile for binary file detection
@dignifiedquire dignifiedquire merged commit 04a68be into karma-runner:master Jan 22, 2016
@mramato mramato deleted the is-binary-file branch January 22, 2016 14:03
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.

None yet

4 participants