Increase coverage#1690
Conversation
📝 Docs previewLast commit e22a7e2 at: https://063fabe3.typertiangolo.pages.dev |
svlandeg
left a comment
There was a problem hiding this comment.
Status update: I'm more or less "done", but I want to review this once more with Claude before I turn this over to Sebastián. Probably next week.
svlandeg
left a comment
There was a problem hiding this comment.
Reviewing this with Claude, capturing some of the most risky decisions in this PR and my reasoning around them. Would be good for @tiangolo to review these points as well.
Issue 1
The
UNSET→Nonecollapse inconsume_valueis the riskiest change
(this is in typer._click.core.py)
I don't actually think this is an issue. I've tried to carefully remove the UNSET logic to keep everything else the same, and Typer tests kept succeeding. Typer has its own logic using Default which should be properly tested. Nevertheless, this is good to keep in the back of our minds in case we see something funny in the future.
Issue 2
value_is_missingstub inParameterbase class: The logic that was here should be verified it's now duplicated in TyperArgument/TyperOption.
Yes, both reimplement that function and I wouldn't expect Typer users to subclass Parameter themselves?
Issue 3
Removed deprecation warning for deprecated parameters
AFAIK, Typer only supports a deprecated flag for commands, not for options/arguments. Something to consider in the future, but I don't think removing the related code in Parameter is currently breaking - it's dead code from Typer's perspective IMO.
Issue 4
check_itersimplification is a behavioral change: (the new)AssertionErroris not caught by Click's error handling and will surface as an unhandled exception rather than aBadParameter(as it was previously).
This refers to this commit and I think it's a valid concern. I reverted this behaviour to raise BadParameter like it was, and wrote a unit test to avoid future regressions: 44cb483
Issue 5
Error message changes break existing tests
There are indeed a few cases where I simplified the Click error code to not use gettext, but also to not have an if/else structure to determine plural. This might change the wording of the error slightly, and might break user's tests if they're testing for specific wording. Should we revert this and reinstate the error messages verbatim?
Issue 6
Context.invoke()was simplified to only support callables; theinvoke(Command, ...)overload was removed. Typer may not use this, but it was part of Click's public API and could affect users who access the underlying context object.
I think this is fine, as we'll probably want to refactor out the context in the future anyway?
Issue 7
NoArgsIsHelpErrorshowmethod writes toerr=True. This is a behavioral change from typical Click behavior where--helpgoes to stdout.
I double checked this, but I think this is a False Positive from Claude's reviewing. err=True was already present in NoArgsIsHelpError.show, as we can see from this diff. There is a new unit test test_no_args_is_help_show in test_others.py that specifically checks the output is in result.stderr. To be sure, I double checked and tested this one on master, where it also succeeds. So as far as I can see, there's no behavioural change.
Issue 8
_depr_flag_needs_valuereference remains inTyperOption
Right, this looks like dead code and has now been removed.
Issue 9
Several TODO: test or remove? comments remain — these should be resolved before merging
I left those in on purpose, these could be cleaned up in future work, in an attempt for this PR to be as least breaking as possible.
Issue 10
make_strwas removed — this was used in_click_resolve_command. The replacementargs[0]assumesargsare always strings, which is fine for CLI usage but removes bytes-path handling that existed for edge cases
I'm not super sure about this one. What type of Typer usage would this edge case be? Oh, let's ask Claude! Now it says
I can't write a meaningful unit test for this because exercising it would require bypassing the public API entirely. This means my concern about removing make_str there was unfounded — it was dead code in the Typer context, and the replacement args[0] is correct.
So actually I think this one is fine as well.
Continue work from #1525, #1601 and #1680, vendoring Click.
After the prep work from previous PRs, this PR contains a lot of work to finish the refactor:
coloramaas dependency for Windows. It's in fact already pulled in bypytestanyway, but I figured explicit is better.It's big to review, but all edits should be relatively local, so the diff can be reviewed as one (and not per-commit). No code has been moved around - it's almost all pure deletions or test additions.
Removed stuff
Sebastián: if there's anything in the list below that we do want to keep as functionality, let me know, then I'll write additional unit tests instead of removing the relevant code.
UNSET(originally introduced in this PR). Typer has its ownDefault(...)to cover these use-cases.FLAG_NEEDS_VALUE,__debug__statements,batch(),protected_args,Parameter.deprecatedforce_readableandforce_writableas those were set toTrueinget_text_stdin,get_text_stdout,get_text_stderranyway_pause_echo&EchoingStdinin_click/testing.py, which wasn't usedThis was all done 100% manually.
Refactored stuff
_click.Command,_click.Parameterand_click.ShellCompleteabstractfrom __future__ import annotations, import directly fromtypingandcollections, remove usage ofgettext, use f-stringsThis was all done 100% manually.
Added tests
Added tests for:
_click.types.py&typer._types.py:reprfunctions mostly added to existing type tutorial testsconvertfunctions intotests/test_type_conversion.pyshell_completionfunctionality intotests/test_completiondir (incl. several new files)tests/test_types.py,tests/test_types_files.py(new)_click._compat.py_AtomicFile stuffintests/test_atomic_file.py(new)tests/test_types_file.py_click._winconsole.py:tests/test_win_console.py(new)_click.termui.pyand_click._termui_impl.py:tests/test_progress_bar.py(new)tests/test_launch.pytests/test_termui.py(new)_click.core.pyand some of the new functionality intyper.core.py:tests/test_core.py(new)_click.utils.py:tests/test_cli/test_help.pytests/test_cli/test_program_name.py(new)tests/test_types_file.py_click.testing.py:make_input_streamstuff intests/test_types_file.pytests/test_termui.py_click.exceptions.py:tests/test_others.py_click.formatting.py&_click._textwrap.py:tests/test_cli/test_help.py_click.parser.pytests/test_cli/test_parser.py(new)_click.shell_completion.pytests/test_completion/test_completion.pyThis was done with AI assistance, micromanaged & thoroughly reviewed by me.
I tried to mostly use public APIs from Typer for these tests, to really showcase user functionality. When this wasn't possible, the relevant code was a candidate for deletion, cf previous section ☝️
Coverage stats
This branch, before this PR:
This branch, after "cleaning" the original Click files:
This branch, after adding unit tests for missing coverage: