Dropping support for --json and --compact and adding support for --output-format #266
Conversation
mayuriesha
requested review from
tarkatronic,
sushantmimani and
rbailey-godaddy
mayuriesha
changed the title
There was a problem hiding this comment.
Just a couple of small changes here. The more I see this, the more I love the decision to make this change!
One thing I'm wondering as I look through this code though -- does it make sense to make an enum? I always hate having to pass around magical string values where typoes could get introduced. What about a types.OutputFormat? I'm very much open to arguments either way on this. 😄
tartufo/cli.py
Outdated
| "--output-format", | ||
| type=click.Choice(["json", "compact", "text"]), | ||
| default="text", | ||
| help="Output can be in json/compact/text format. By default the output will be in text format.", |
There was a problem hiding this comment.
I really like the description you added to the README.md for this option -- can you put that in here?
tartufo/scanner.py
Outdated
| @@ -68,12 +68,12 @@ def __init__( | |||
| self.chunk = chunk | |||
| self.logger = logging.getLogger(__name__) | |||
|
|
|||
| def as_dict(self, compact=False) -> Dict[str, Optional[str]]: | |||
| def as_dict(self, output_format=None) -> Dict[str, Optional[str]]: | |||
There was a problem hiding this comment.
I think it makes the most sense to keep this method the way it was, using a boolean. Since the other values of output_format mean nothing to this, a boolean seems cleanest.
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 #253
What's new?
--jsonand--compactoptions and instead uses single flag--output-format json/compact/textto support different output formats