Conversation
tabcmd Classic's `--encrypt` on `createextracts` takes an explicit yes/no value; tabcmd 2 treated it as a bare boolean flag, silently ignoring any following value. Scripts that pass `--encrypt no` on Classic ended up encrypting on tabcmd 2 because argparse consumed the "no" as a positional or bailed. Make --encrypt accept an optional yes|no|true|false argument (case-insensitive). Bare `--encrypt` still means True; omitted means False; explicit `--encrypt no` means False. This preserves current tabcmd 2 default behavior while matching Classic syntax.
Classic tabcmd's CreateExtracts.java:79-101 accepts exactly `yes|y|no|n` (case-insensitive) on --encrypt and rejects anything else. This PR was dropping the `y`/`n` shortcuts, which breaks any ported Classic script that used them. Keeping Classic's set intact is the standing rule — tabcmd 2 may be more forgiving than Classic, but must not narrow. Add `y` and `n` to the accepted set (both directions). tabcmd 2 now accepts the union `yes|y|no|n|true|false|1|0`, case-insensitive. Rewrite the source comment to cite the Classic source-of-truth path in the monolith rather than the incorrect earlier claim that Classic accepted "yes/no/true/false". Add two tests pinning `y`→True and `n`→False so a future refactor can't quietly drop them again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Add regression tests for the supported 1 and 0 aliases and update the help text to reflect all accepted values.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates createextracts --encrypt to accept Classic-compatible boolean values while preserving bare-flag behavior.
Changes:
- Supports yes/no, y/n, true/false, and 1/0 values.
- Adds parser coverage for accepted and rejected inputs.
File summaries
| File | Description |
|---|---|
tests/parsers/test_parser_create_extracts.py |
Tests encryption argument behavior. |
tabcmd/execution/global_options.py |
Implements optional encryption-value parsing. |
Review details
Suppressed comments (1)
tabcmd/execution/global_options.py:125
- The parser now explicitly supports the
1/0aliases (and the PR description calls out0), but the added tests cover onlyy/nandtrue/false. Please add regression cases for both numeric spellings so this part of the accepted interface is protected from future changes.
if lowered in ("yes", "y", "true", "1"):
return True
if lowered in ("no", "n", "false", "0"):
return False
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const=True, | ||
| default=False, | ||
| type=_parse_yes_no, | ||
| metavar="yes|no", |
This file contains hidden or 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
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.
Motivation
tabcmd Classic's
--encryptoncreateextractstakes an explicityesornovalue. tabcmd 2 treated it as a bare boolean flag,silently ignoring any following value. Scripts passing
--encrypt noon Classic didn't fail on tabcmd 2 but produced the wrong behavior
(they either bailed on argparse or silently kept encryption on).
Behavior change
For users:
--encryptaccepts an optionalyes|no|true|falseargument (case-insensitive).
--encryptstill meansTrue(tabcmd 2 syntax).False.--encrypt no(orfalse/0) now meansFalse.Preserves tabcmd 2's current default while accepting Classic's
yes/no syntax.
Test plan
tests/parsers/test_parser_create_extracts.py— 8 new tests:omitted / bare / yes / no / true / false / case-insensitive /
bad-value-rejected
test_parser_create_extracts.pysuite: 15 passed🤖 Generated with Claude Code
Source
Classic tabcmd's
--encrypthandler (app-tabcmd/src/.../commands/CreateExtracts.java:79-101) accepts exactlyyes | y | no | n(case-insensitive) and rejects anything else. Latest push (fb14edc) restoresy/n— the Classic set — to the accepted values so ported scripts using the shortcuts keep working. Full tabcmd 2 set is nowyes | y | no | n | true | false | 1 | 0: Classic's set intact, withtrue/false/1/0added on top as a forgiving superset for users typing boolean-shaped values.Updated test coverage: 10 argparse tests (omitted / bare / yes / no / y / n / true / false / case-insensitive / bad-value-rejected).