Exclusion reasons #286
Exclusion reasons #286
Conversation
mayuriesha
requested review from
tarkatronic,
sushantmimani and
rbailey-godaddy
tartufo/scanner.py
Outdated
| signatures = self.config_data.get("exclude_signatures", None) | ||
| if signatures: | ||
| warnings.warn( | ||
| "--exclude-signatures will be deprecated. Make sure all the exclusions are moved to " |
There was a problem hiding this comment.
| "--exclude-signatures will be deprecated. Make sure all the exclusions are moved to " | |
| "--exclude-signatures has been deprecated and will be removed in a future version. Make sure all the exclusions are moved to " |
tartufo/scanner.py
Outdated
| @@ -149,6 +150,8 @@ class ScannerBase(abc.ABC): # pylint: disable=too-many-instance-attributes | |||
| global_options: types.GlobalOptions | |||
| logger: logging.Logger | |||
| _scan_lock: threading.Lock = threading.Lock() | |||
| _excluded_findings: tuple = () | |||
There was a problem hiding this comment.
| _excluded_findings: tuple = () | |
| _excluded_findings: Tuple[str, ...] = () |
tartufo/scanner.py
Outdated
| def config_data(self, data) -> MutableMapping[str, Any]: | ||
| self._config_data = data | ||
| return self._config_data |
There was a problem hiding this comment.
| def config_data(self, data) -> MutableMapping[str, Any]: | |
| self._config_data = data | |
| return self._config_data | |
| def config_data(self, data: MutableMapping[str, Any]) -> None: | |
| self._config_data = data |
Returning data from a setter in Python is generally not a valuable operation, because there's no way to access it. You would need something like...
new_data = scanner.config_data = dataWhich is just confusing. And probably won't even work the way you expect.
You can find a bit more of an explanation here, if you're interested: https://stackoverflow.com/a/16615910
There was a problem hiding this comment.
This is great @mayuriesha, thank you!
As we have continued talking about this feature, and #257, I can't help but wonder if I was wrong in thinking we should allow both old-style and new-style config to exist side-by-side. The more I think about it, the more I think I was wrong. We're already forcing so much config change and churn on our users, why force them to learn new names for the config entries as well, especially names that might not make as much sense?
I hate to suggest a change to this so late. But I wonder if this, and #257, are cases where we should operate similar to what we did with exclude-entropy-patterns in 2.x. Either you use all old-style config, or all new-style config. I am honestly open to going either direction, so I'd love some more thoughts on it! And of course we can always merge this PR as-is, to get it checked off, and update it in the future if we decide to go the other way!
Thank you again for the hard work @mayuriesha!
To help us get this pull request reviewed and merged quickly, please be sure to include the following items:
PR Type
What kind of change does this PR introduce?
Backward Compatibility
Is this change backward compatible with the most recently released version? Does it introduce changes which might change the user experience in any way? Does it alter the API in any way?
Issue Linking
closes #202
What's new?