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

Fix "pre-commit hook removes plugins from baseline" #40

Merged
merged 6 commits into from
Jun 22, 2018

Conversation

KevinHock
Copy link
Collaborator

This fixes the issue @LouisTrezzini reported in #32 where we would not read in or output plugin information when reading a baseline from the pre-commit hook code.

Copy link
Collaborator Author

@KevinHock KevinHock left a comment

Choose a reason for hiding this comment

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

👍


plugins = []
for plugin in data['plugins_used']:
plugin_classname = plugin.pop('name')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm so glad I finally got to use pop (cc @omergunal)

@KevinHock KevinHock requested a review from domanchi June 22, 2018 01:10
Copy link
Contributor

@domanchi domanchi left a comment

Choose a reason for hiding this comment

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

My main question is regarding nargs='?' and those repercussions through the codebase.

Otherwise, lgtm.

if not all(key in data for key in (
'exclude_regex',
'results',
'plugins_used'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is gonna be a "breaking" change, because plugins_used was only introduced "recently". Then again, as long as we bundle this with #38 before wrapping it up as a version, it should be OK.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the requirement of version in my merge with master, which definitely breaks (3 months ago plugins_used was added vs. version just added today.) 👍

@@ -210,13 +210,13 @@ def _add_custom_limits(self):
self.parser.add_argument(
'--base64-limit',
type=self._argparse_minmax_type,
nargs=1,
nargs='?',
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean to have --base64-limit without any limit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I follow, do you mean if I do python -m detect_secrets.main --base64-limit --scan test_secrets/? It uses the default.

@@ -145,7 +141,7 @@ def _convert_sensitivity_values_to_class_tuple(sensitivity_values):
This way, we can initialize the class with <plugin_class_name>(<value>)

Example:
>>> [ ('HexHighEntropyString', 3), ('PrivateKeyDetector', true), ]
>>> [ ('HexHighEntropyString', {'hex_limit': 3}), ('PrivateKeyDetector', {'private_key_detector': true}), ]
Copy link
Contributor

Choose a reason for hiding this comment

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

please shorten this line. =)

Maybe,

>>> [
...    ('HexHighEntropyString', {'hex_limit': 3}),
...    ('PrivateKeyDetector', {'private_key_detector': true}),
... ]

?

To make it easier, I will adjust E501 settings for the future.

@KevinHock KevinHock merged commit d041785 into master Jun 22, 2018
@KevinHock KevinHock deleted the why_does_baseline_not_work_anymore branch June 23, 2018 18:49
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

2 participants