click.prompt typing clarifications / improvements#3407
click.prompt typing clarifications / improvements#3407AndreasBackx wants to merge 5 commits intopallets:mainfrom
Conversation
|
The question isn't about the ideal type, it's what the other developers already rely on. We're in Hyrum's Law territory here: Click is too entranched in the ecosystem that we have users depending on behaviors we never strictly specified. Let's pin the current runtime behavior with tests before tightening anything. Does option 3 breaks the actual unittests? Can you come up with tests that encode the actually behaviour we allow but we don't account for? |
|
@kdeldycke I've made changes to the typing, though it has lead to the introduction of a new generic type being added to However, to make it easier for people and not require custom |
Yeah that looks good! And indeed: it passes on Python 3.13 without issues.
If it would be my project I would say yes. But not sure what's our policy for Click. So thanks for asking! @davidism, @Rowlando13, @ThiefMaster, any opinion on adding In the meantime @AndreasBackx add that dependency to this PR so you can demonstrate that it make your changes pass on all Pythons supported by Click. Could also help other maintainers form an opinion by reading the actual impact of your changes. |
This improves the typing of
click.prompt. However there is still an opening question / issue...Brief summary
The current code makes the following change to the public API of
click.prompt:def prompt( text: str, - default: t.Any | None = None, + default: V | None = None, hide_input: bool = False, confirmation_prompt: bool | str = False, - type: ParamType[t.Any] | t.Any | None = None, - value_proc: t.Callable[[str], t.Any] | None = None, + type: ParamType[V] | V | None = None, + value_proc: t.Callable[[str], V] | None = None, prompt_suffix: str = ": ", show_default: bool | str = True, err: bool = False, show_choices: bool = True, -) -> t.Any: +) -> V:This changes the behaviour in that you can no longer pass types that aren't the same type as the type you expect at the end. For example:
Would no longer work under the current implementation because
"100"is not of the typeint. In the old implementation this worked because default values still were passed asint("100")before being returned. The remaining question is then what behaviour we should go for.You could say, "why" not keep the current implementation? Well we could, but it's not actually typed correctly and actually allows behaviour we don't account for. That behaviour is that
value_proc: t.Callable[[str], V] | None = Noneaccepts onlystrto convert to the target type. So technically you shouldn't be allowed to passAnyto it. However....ParamType.__call__, which makesParamTypealso a callable, is used to convert thedefaultto the targettypeas well and that acceptsAny, notstr. So there's not really a strict contract anywhere.To make the decision on this PR easier I propose the following options:
1. Restrict
defaultto same type (current implementation)typeindefault.defaulttovalue_procor the constructor oftype.def prompt( text: str, - default: t.Any | None = None, + default: V | None = None, hide_input: bool = False, confirmation_prompt: bool | str = False, - type: ParamType[t.Any] | t.Any | None = None, - value_proc: t.Callable[[str], t.Any] | None = None, + type: ParamType[V] | V | None = None, + value_proc: t.Callable[[str], V] | None = None, prompt_suffix: str = ": ", show_default: bool | str = True, err: bool = False, show_choices: bool = True, -) -> t.Any: +) -> V:2. Public API typing change (or correction) with unlikely users
typeindefaultand alsostr.defaulttype is different from the type oftype, pass it tovalue_procor the constructor oftypeto convert it.ParamType.__call__also onyl acceptstr, notAny.def prompt( text: str, - default: t.Any | None = None, + default: V | str | None = None, hide_input: bool = False, confirmation_prompt: bool | str = False, - type: ParamType[t.Any] | t.Any | None = None, - value_proc: t.Callable[[str], t.Any] | None = None, + type: ParamType[V] | V | None = None, + value_proc: t.Callable[[str], V] | None = None, prompt_suffix: str = ": ", show_default: bool | str = True, err: bool = False, show_choices: bool = True, -) -> t.Any: +) -> V:@t.overload def __call__( self, value: None, param: Parameter | None = None, ctx: Context | None = None, ) -> None: ... @t.overload def __call__( self, - value: t.Any, + value: str, param: Parameter | None = None, ctx: Context | None = None, ) -> ParamTypeValue: ... def __call__( self, - value: t.Any, + value: str, param: Parameter | None = None, ctx: Context | None = None, ) -> ParamTypeValue | None: if value is not None: return self.convert(value, param, ctx) return None def convert( self, - value: t.Any, + value: str, param: Parameter | None, ctx: Context | None, ) -> ParamTypeValue:3. Change
value_procto acceptAnyAnyindefault, keeping the same accepted type as before.defaulttype is different from the type oftype, pass it tovalue_procor the constructor oftypeto convert it.def prompt( text: str, - default: t.Any | None = None, + default: V | t.Any | None = None, hide_input: bool = False, confirmation_prompt: bool | str = False, - type: ParamType[t.Any] | t.Any | None = None, - value_proc: t.Callable[[str], t.Any] | None = None, + type: ParamType[V] | V | None = None, + value_proc: t.Callable[[t.Any], V] | None = None, prompt_suffix: str = ": ", show_default: bool | str = True, err: bool = False, show_choices: bool = True, -) -> t.Any: +) -> V:My preference?
Even though I've implemented the first option, I think the second option makes the most sense. I don't think there's a usecase for accepting beyond
strfor parsing.