Remove deprecated code #433
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rbailey-godaddy
requested changes
Jan 13, 2023
There was a problem hiding this comment.
Nits.
Mostly, beef up the changelog entry for people who have forgotten what things we dropped... The rest is complaints about the way you specified tuples in various test methods. Just specify the tuples like before, but with dict members instead of str members:
my_tuple = ({"my": "dict"},)
CHANGELOG.md
Outdated
| @@ -2,6 +2,8 @@ vX.X.X - DD MMM YYYY | |||
| -------------------- | |||
|
|
|||
| Features: | |||
| * [#433](https://github.com/godaddy/tartufo/pull/433) - Dropped support for deprecated flags from v3.0 | |||
There was a problem hiding this comment.
In this context, and in the interest of visibility, I would explicitly list the removed flags here again... for people who only read the CHANGELOG and not the error messages. ;-)
tests/test_base_scanner.py
Outdated
Comment on lines
198
to
200
| self.options.include_path_patterns = tuple( | ||
| [{"path-pattern": "foo", "reason": "Testing exclude path pattern"}] | ||
| ) |
There was a problem hiding this comment.
This works but I had to read it 3 times and then try it myself to convince myself it did what was needed. How about:
Suggested change
| self.options.include_path_patterns = tuple( | |
| [{"path-pattern": "foo", "reason": "Testing exclude path pattern"}] | |
| ) | |
| self.options.include_path_patterns = ( | |
| {"path-pattern": "foo", "reason": "Testing exclude path pattern"}, | |
| ) |
tests/test_base_scanner.py
Outdated
Comment on lines
222
to
224
| self.options.exclude_path_patterns = tuple( | ||
| [{"path-pattern": "foo", "reason": "Testing exclude path pattern"}] | ||
| ) |
There was a problem hiding this comment.
Ditto above. The only reason I comment is that it isn't obvious to the casual reader that you're getting a tuple consisting of a single dict vs a tuple consisting of a single list
tests/test_base_scanner.py
Outdated
Comment on lines
292
to
294
| self.options.exclude_signatures = tuple( | ||
| [{"signature": "foo", "reason": "Testing exclude signature"}] | ||
| ) |
tests/test_base_scanner.py
Outdated
Comment on lines
303
to
305
| self.options.exclude_signatures = tuple( | ||
| [{"signature": "foo", "reason": "Testing exclude signature"}] | ||
| ) |
tests/test_git_repo_scanner.py
Outdated
Comment on lines
79
to
80
| self.global_options.include_path_patterns = tuple( | ||
| [{"path-pattern": "foo/", "reason": "Inclusion reason"}] |
There was a problem hiding this comment.
Suggested change
| self.global_options.include_path_patterns = tuple( | |
| [{"path-pattern": "foo/", "reason": "Inclusion reason"}] | |
| self.global_options.include_path_patterns = ( | |
| {"path-pattern": "foo/", "reason": "Inclusion reason"}, |
tests/test_git_repo_scanner.py
Outdated
Comment on lines
104
to
105
| self.global_options.exclude_path_patterns = tuple( | ||
| [{"path-pattern": "bar/", "reason": "Exclusion reason"}] |
There was a problem hiding this comment.
Suggested change
| self.global_options.exclude_path_patterns = tuple( | |
| [{"path-pattern": "bar/", "reason": "Exclusion reason"}] | |
| self.global_options.exclude_path_patterns = ( | |
| {"path-pattern": "bar/", "reason": "Exclusion reason"}, |
tests/test_git_repo_scanner.py
Outdated
Comment on lines
132
to
133
| self.global_options.exclude_signatures = tuple( | ||
| [{"signature": "bar", "reason": "Reason to exclude signature"}] |
There was a problem hiding this comment.
Suggested change
| self.global_options.exclude_signatures = tuple( | |
| [{"signature": "bar", "reason": "Reason to exclude signature"}] | |
| self.global_options.exclude_signatures = ( | |
| {"signature": "bar", "reason": "Reason to exclude signature"}, |
sushantmimani
requested changes
Jan 13, 2023
tests/test_git_repo_scanner.py
Outdated
| test_scanner = scanner.GitRepoScanner( | ||
| self.global_options, self.git_options, "." | ||
| ) | ||
| self.assertEqual(test_scanner.excluded_signatures, ("bar/",)) | ||
|
|
||
| def test_error_is_not_raised_when_two_styles_signatures_are_configured(self): | ||
| def test_error_is_raised_when_two_styles_signatures_are_configured(self): |
There was a problem hiding this comment.
Suggested change
| def test_error_is_raised_when_two_styles_signatures_are_configured(self): | |
| def test_error_is_raised_when_string_signature_is_used(self): |
tests/test_git_repo_scanner.py
Outdated
| test_scanner = scanner.GitRepoScanner( | ||
| self.global_options, self.git_options, "." | ||
| ) | ||
| self.assertEqual(test_scanner.included_paths, [re.compile("bar/")]) | ||
|
|
||
| def test_error_is_not_raised_when_two_styles_included_paths_are_configured(self): | ||
| def test_error_is_raised_when_two_styles_included_paths_are_configured(self): |
There was a problem hiding this comment.
Suggested change
| def test_error_is_raised_when_two_styles_included_paths_are_configured(self): | |
| def test_error_is_raised_when_string_included_path_is_used(self): |
tests/test_git_repo_scanner.py
Outdated
| test_scanner = scanner.GitRepoScanner( | ||
| self.global_options, self.git_options, "." | ||
| ) | ||
| self.assertEqual(test_scanner.excluded_paths, [re.compile("bar/")]) | ||
|
|
||
| @mock.patch("tartufo.scanner.GitScanner.filter_submodules", mock.MagicMock()) | ||
| def test_error_is_not_raised_when_two_styles_excluded_paths_are_configured(self): | ||
| def test_error_is_raised_when_two_styles_excluded_paths_are_configured(self): |
There was a problem hiding this comment.
Suggested change
| def test_error_is_raised_when_two_styles_excluded_paths_are_configured(self): | |
| def test_error_is_raised_when_string_exclude_path_is_used(self): |
tests/test_git_repo_scanner.py
Outdated
| self.global_options.exclude_signatures = [ | ||
| "foo/", | ||
| {"signature": "bar/", "reason": "path pattern"}, | ||
| ] | ||
| test_scanner = scanner.GitRepoScanner( | ||
| self.global_options, self.git_options, "." | ||
| ) | ||
| self.assertCountEqual(test_scanner.excluded_signatures, ("foo/", "bar/")) | ||
| with self.assertRaisesRegex( | ||
| ConfigException, "signature is illegal in exclude-signatures" |
There was a problem hiding this comment.
Suggested change
| ConfigException, "signature is illegal in exclude-signatures" | |
| ConfigException, "str signature is illegal in exclude-signatures" |
|
|
||
| def evaluate_entropy_string( | ||
| self, | ||
| chunk: types.Chunk, | ||
| line: str, | ||
| string: str, | ||
| min_entropy_score: float, | ||
| backwards_compatibility_prefix: Optional[str], |
There was a problem hiding this comment.
Docstring and documentation should be updated
rbailey-godaddy
approved these changes
Jan 13, 2023
sushantmimani
approved these changes
Jan 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
What's new?