Easy plugin development #248
Conversation
This removes the hard-coded PluginDescriptors, as well as the associated hard-coded tests that needed to be changed with the addition of a new plugin. In doing so, this also addresses #146.
| if line: | ||
| break | ||
| else: | ||
| raise NotImplementedError('Plugins must declare a docstring.') |
| return mapping | ||
| return { | ||
| value.secret_type: key | ||
| for key, value in import_plugins().items() |
There was a problem hiding this comment.
Super nit: I know it says plugin classname above, but I liked when you did e.g.
for name, plugin in import_plugins().items()
in other places, as it was more immediately clear. / Consistency nit.
| from testing.factories import secrets_collection_factory | ||
| from testing.mocks import Any | ||
| from testing.mocks import mock_printer | ||
| from testing.util import uncolor | ||
|
|
||
|
|
||
| def get_list_of_plugins(include=None, exclude=None): |
There was a problem hiding this comment.
Super nit: One might expect include to mean 'only include', maybe customized and excluded might be more intuitive. Alternatively maybe a docstring saying include is only necessary for plugins w/out the defaults.
| if not extra: | ||
| extra = {} | ||
|
|
||
| longest_name_length = max([ |
| if name.endswith('Detector'): | ||
| name = name[:-len('Detector')] | ||
|
|
||
| # turn camel case into hyphenated strings |
There was a problem hiding this comment.
Super consistency nit: All our other comments start cap'd
| @@ -14,7 +14,7 @@ commands = | |||
| coverage run -m pytest tests -v | |||
| coverage report --show-missing --include=tests/* --fail-under 100 | |||
| # This is so that we do not regress unintentionally | |||
| coverage report --show-missing --include=detect_secrets/* --omit=detect_secrets/core/audit.py,detect_secrets/core/secrets_collection.py,detect_secrets/main.py,detect_secrets/plugins/common/ini_file_parser.py --fail-under 100 | |||
| coverage report --show-missing --include=detect_secrets/* --omit=detect_secrets/core/audit.py,detect_secrets/core/secrets_collection.py,detect_secrets/main.py,detect_secrets/plugins/common/ini_file_parser.py --fail-under 99 | |||
There was a problem hiding this comment.
I am not a 💯 fanatic, and okay with regressing coverage. My only issue with not separating out the file(s) is that, now when someone makes a PR, including this one, it's not that clear what isn't covered. pragma: no coverage is another option, some people feel it's "lying" (I don't) but it definitely helps with the above.
As an aside, past code like de7fbd2#diff-895e31d74e6f4c86ab33d77ef2ff05ed and 20d1921#diff-f56910fb7243ad4ed59cd5cf2e1af814 are semi-decent examples of lack of test coverage making things unclear as to why they were written.
|
|
||
| Be sure to add your changes to the `README.md` and `CHANGELOG.md` so that | ||
| it will be easier for maintainers to bump the version and for other | ||
| downstream consumers to get the latest information about plugins available. | ||
|
|
||
| ### Tips | ||
|
|
||
| - There should be a total of three modified files in a minimal new plugin: the |
There was a problem hiding this comment.
Would it's corresponding test, be equivalent to what you mean?
* Add script to build new docker images * Use baseline * Add dockerignore * Fix dockerfile dep * Different way to tag image
* Add script to build new docker images * Use baseline * Add dockerignore * Fix dockerfile dep * Different way to tag image
Addresses #154, by reducing the number of files that need to be changed when adding a plugin from 9 (ref) to 3.
This also accidentally addresses #146, because while modifying tests, the new tests broke on this bug (which is what tests are for).