Skip to content

Updating exclude signatures to use either one of the formats #290

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Dec 9, 2021
Merged

Updating exclude signatures to use either one of the formats #290

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
28e6b48
Updating exclude signatures to use either one of the formats
mayuriesha Dec 3, 2021
ed6c79e
Fixing mypy issues with data type incompatibility by using scanners e…
mayuriesha Dec 3, 2021
d492f38
Addressing review on deprecation messages
mayuriesha Dec 6, 2021
19f881a
Fixing unit test
mayuriesha Dec 6, 2021
a8c19a3
Updating message
mayuriesha Dec 6, 2021
d327626
Processing the signatures added in the config as well
mayuriesha Dec 7, 2021
dd0da0b
Adding comments for exclusions
mayuriesha Dec 7, 2021
d1ad2f5
Apply suggestions from @tarkatronic code review
rbailey-godaddy Dec 8, 2021
58422c8
Fix problems caused by latest fixes & incorporate @rbailey-godaddy ch…
tarkatronic Dec 8, 2021
987a002
Fix up `excluded_signatures` processing
rscottbailey Dec 9, 2021
6e36048
pylint fix
rscottbailey Dec 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions tartufo/scanner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ class ScannerBase(abc.ABC): # pylint: disable=too-many-instance-attributes
global_options: types.GlobalOptions
logger: logging.Logger
_scan_lock: threading.Lock = threading.Lock()
_excluded_signatures: Tuple[str, ...] = ()
_excluded_signatures: Optional[Tuple[str, ...]] = None
_config_data: MutableMapping[str, Any] = {}

def __init__(self, options: types.GlobalOptions) -> None:
Expand Down Expand Up @@ -349,19 +349,17 @@ def config_data(self, data: MutableMapping[str, Any]) -> None:
self._config_data = data

@cached_property
def excluded_signatures(self) -> tuple:
if len(self._excluded_signatures) == 0:
signatures = list(self.global_options.exclude_signatures or ())
exclude_signatures = list(self.config_data.get("exclude_signatures", ()))
def excluded_signatures(self) -> Tuple[str, ...]:
if self._excluded_signatures is None:
signatures = (tuple(self.global_options.exclude_signatures) or ()) + (
tuple(self.config_data.get("exclude_signatures", ()))
)
try:
signatures = signatures + [
x for x in exclude_signatures if x not in signatures
]
signatures = [
signatures = tuple(
signature["signature"]
for signature in cast(List[Dict[str, str]], signatures)
]
self._excluded_signatures = tuple(cast(List[str], signatures))
for signature in cast(Tuple[Dict[str, str]], signatures)
)
self._excluded_signatures = tuple(set(signatures))
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes no sense to me. Why use a tuple comprehension, and then convert it to a set, and then convert it back to a tuple -- instead of starting with a set comprehension and then converting it once? If we're worried about mutating the type of signatures, then just eliminate the intermediate variable and store directly into the destination.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah... I had very similar thoughts. And had your words echoing in my head as I did it. I just didn't want to have to think about all the other places that might be depending on a tuple vs a set. In theory it shouldn't matter. But... you never know.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rbailey-godaddy Since it's already getting to dinner time, I'm happy to leave this stewing overnight in case you want to take a moment in the morning to un-thrashify this code. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

But, also want to keep in mind that we're more than a week overdue for 3.0 now, so... limiting scope is good.

Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed a commit that makes me moderately happy. (Well, two because pylint is so anal.) For "proper" usage, this makes no changes that are visible outside of the modified property function. Changes in behavior:

  • If you mix old and new style specifications, it now works. (Fixed unit test to confirm this.) This does not affect whether or not you can -- or should -- actually mix styles in your configuration files, only that we aren't artificially restricting things ourselves.
  • If you pass new-style entry that is missing signature key, it tells you that explicitly. Previously it would raise an unhandled KeyError.
  • If you (somehow) pass an entry that's neither old-style nor new-style, it tells you that explicitly. Previously such alien input would be treated as "old-style" and inserted directly into the return value tuple, where it probably would be ignored.

As a matter of taste, "tells you explicitly" means "raises meaningful exception and aborts" rather than "whines, ignores the erroneous entry, and continues." I feel if you have b0rked configuration files, you should fix them.

Also, I marked all of the conversations above "resolved" where they discussed outdated code.

return self._excluded_signatures
except TypeError:
warnings.warn(
Expand All @@ -372,8 +370,7 @@ def excluded_signatures(self) -> tuple:
DeprecationWarning,
)
try:
signatures = list(set(signatures + exclude_signatures))
self._excluded_signatures = tuple(cast(List[str], signatures))
self._excluded_signatures = tuple(set(cast(Tuple[str], signatures)))
except TypeError as exc:
raise types.ConfigException(
f"Combination of old and new format of exclude-signatures is not supported.\n{exc}"
Expand Down
6 changes: 3 additions & 3 deletions tests/test_git_repo_scanner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,14 @@ def test_extra_exclusions_get_added(self, mock_load: mock.MagicMock):
def test_extra_signatures_get_added(self, mock_load: mock.MagicMock):
mock_load.return_value = (
self.data_dir / "pyproject.toml",
{"exclude_signatures": ["foo", "bar"]},
{"exclude_signatures": ["foo"]},
)
self.global_options.exclude_signatures = ("foo", "bar")
self.global_options.exclude_signatures = ("bar",)
test_scanner = scanner.GitRepoScanner(
self.global_options, self.git_options, str(self.data_dir)
)
test_scanner.load_repo("../tartufo")
self.assertEqual(sorted(test_scanner.excluded_signatures), ["bar", "foo"])
self.assertCountEqual(test_scanner.excluded_signatures, ["bar", "foo"])


class FilterSubmoduleTests(ScannerTestCase):
Expand Down