-
Notifications
You must be signed in to change notification settings - Fork 479
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
Improve handle of un-scannable files #176
Improve handle of un-scannable files #176
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me, thank you for making this! ✨ 🍰 ✨
Left a nit for the TravisCI build fix
@@ -200,7 +201,8 @@ def scan_file(self, filename, filename_key=None): | |||
|
|||
if os.path.islink(filename): | |||
return False | |||
|
|||
if (os.path.splitext(filename)[1] in IGNORED_FILE_EXTENSIONS): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just writing this down for posterity.)
We could fill two needs with one deed if we put this in _results_accumulator
i.e.
if os.path.splitext(filename)[1] in IGNORED_FILE_EXTENSIONS:
return
This is because for regular detect-secrets
, the control flow goes scan_file
->_extract_secrets_from_file
->_results_accumulator
. And for detect-secrets-server
the control flow goes scan_diff
->_results_accumulator
.
However, let's leave the location as-is for this PR, it's better 😀 👍 . The current location where we filter out ignored file extensions in detect-secrets-server
, is a lot earlier on, (before scan_diff
ever gets called) and better performance-wise. So it's not worth it to make it super DRY at the cost of performance IMO.
Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com> Fix style Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com> Improve style Signed-off-by: Daniel González Lopes <danielgonzalezlopes@gmail.com>
d5fea6f
to
5cb6074
Compare
Sorry for missing the style! Squashed my commits for cleaning the PR. Also, the test failing seems to be the flaky one #167. |
++, thanks :) Re-running Travis now, will merge when it passes |
@@ -200,7 +201,8 @@ def scan_file(self, filename, filename_key=None): | |||
|
|||
if os.path.islink(filename): | |||
return False | |||
|
|||
if os.path.splitext(filename)[1] in IGNORED_FILE_EXTENSIONS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turned out to be a sneaky bug I found today, splitext
will always have .extension
in it, and so we were checking if e.g. .svg
== svg
. I'll push a fix 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we made the same mistake in -server too
Signed-off-by: Daniel González Lopes danielgonzalezlopes@gmail.com
Now using Set data type as said on #141.