Updating exclude signatures to use either one of the formats #290
Conversation
mayuriesha
requested review from
tarkatronic,
sushantmimani and
rbailey-godaddy
…xcluded_signatures instead of global_options.exclude_signatures
There was a problem hiding this comment.
I think this has the same basic problem as the other PR, in the sense that we want to process the global and local lists separately so they are allowed to have different representations. You should be able to use the same approach to disconnect them.
And, although not strictly necessary, I would advocate changing the return from a tuple to a set (but leave it alone if @tarkatronic or anybody else thinks this would be problematic).
|
@rbailey-godaddy I have created an issue to address the last few review comment as part of another PR after 3.0 is released |
There was a problem hiding this comment.
This layers on top of @tarkatronic 's comments, mostly in eliminating dangling use of removed variables and preserving exception semantics.
Co-authored-by: Joey Wilhelm <tarkatronic@gmail.com>
tartufo/scanner.py
Outdated
| 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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. 😄
There was a problem hiding this comment.
But, also want to keep in mind that we're more than a week overdue for 3.0 now, so... limiting scope is good.
There was a problem hiding this comment.
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.
* No changes to input or output types * Remove intermediate churning and explicit casting * Sanely report missing new-style key (by aborting rather than skipping the malformed expression -- matter of taste) * Sanely report bizarre input (neither new nor old style) * As side effect, remove prohibition against mixing styles, although whether or not TOML support will puke if you try it is another story * Unit test updated to verify mix-n-match behavior
| for signature in tuple( | ||
| self.global_options.exclude_signatures or [] | ||
| ) + tuple(self.config_data.get("exclude_signatures", [])): |
There was a problem hiding this comment.
>>> () + []
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: can only concatenate tuple (not "list") to tuple
>>>Careful of the mixture of lists and tuples here. That was part of the cause for the previous type thrashing. 😄
There was a problem hiding this comment.
That's why both of them are being explicitly converted from lists to tuples here. Also, I played with this extensively last night and python definitely thinks both self.global_options.exclude_signatures and self.config_data["exclude_signatures"] are lists, which is why I used empty lists for defaults -- to maintain type consistency.
This is strangely bizarre in the sense that self.global_options.exclude_signatures pretty clearly appears to be defined as a tuple rather than a list, but if you try removing either of the tuple conversions you get errors complaining that they are lists and that you aren't allowed to concatenate lists this way (or combine a list and a tuple, as the case may be).
I didn't have enough ambition to trace down where the actual content is coming from to understand why mypy isn't whining about some assignment somewhere that is putting a list into a tuple type... Unless it's happening under the covers in click.
Clean up `included_paths` and `excluded_paths` with basically the same logic used for godaddy#290.
… table of tables (#289) * Supporting table of contents for include and exclude path patterns * Updating unit tests and documentation * Updating logic for inlcude and exclude paths to use two different formats * Adding try catch * Minor wording changes * Updating Changelog * Addressing review comments on deprecation message * Processing the patterns added in the config as well * Updating unit tests to verify patterns are processed from config and global options * Updating include-path-patterns * Update path processing Clean up `included_paths` and `excluded_paths` with basically the same logic used for #290. * reformat merged test file with black Co-authored-by: Scott Bailey <scott.bailey@godaddy.com> Co-authored-by: Scott Bailey <72747501+rbailey-godaddy@users.noreply.github.com>
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?