Field.number(kind=...) supports any function that returns a number

[rouge8]

For example, in our internal codebase, we have a function:

def decimal_from_str(value: str, parens_negate: bool = False) -> Decimal:
    ...

amount = Field.number(kind=decimal_from_str)

[glyph]

SupportsFloat is a weird type for this (presumably in the context of Decimal it's quite intentionally not being float-ified), but it's more correct than Type given how it is used, so we should probably merge this for now.

[rouge8]

Hmm, I didn't notice mypy isn't happy with this. We might need our own protocol with __float__ and the comparison operators. @glyph does that sound reasonable?

[glyph]

Oops, looks like the mypy failure here is real. @rouge8 looks like SupportsFloat is sufficiently weird that it's actually wrong. Can you pick a type here that at least makes mypy happy? The best option here would be to write a numeric protocol, set it as a bound on a TypeVar, then have minimum, maximum, and kind all reference that var so as to ensure consistency.

[glyph]

Hmm, I didn't notice mypy isn't happy with this. We might need our own protocol with __float__ and the comparison operators. @glyph does that sound reasonable?

Is __float__ really even necessary here? It feels like just the comparison would do it?

[rouge8]

Hmm, I didn't notice mypy isn't happy with this. We might need our own protocol with __float__ and the comparison operators. @glyph does that sound reasonable?

Is __float__ really even necessary here? It feels like just the comparison would do it?

That's a good point. The other mypy issue is str vs AnyStr, which I opened #708 for

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.07%. Comparing base (0f3597e) to head (030e604).

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk     #709   +/-   ##
=======================================
  Coverage   99.07%   99.07%           
=======================================
  Files          46       46           
  Lines        3986     4002   +16     
  Branches      531      540    +9     
=======================================
+ Hits         3949     3965   +16     
  Misses         23       23           
  Partials       14       14           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rouge8]

@glyph updated with a _Numeric protocol! I tried setting it as a bound on a TypeVar and using that for minimum, maximum, and kind, but mypy complains and I couldn't figure out how to make it happy:

diff --git a/src/klein/_form.py b/src/klein/_form.py
index da93a33..04de050 100644
--- a/src/klein/_form.py
+++ b/src/klein/_form.py
@@ -56,6 +56,9 @@ class _Numeric(Protocol[_T]):
         ...
 
 
+_N = TypeVar("_N", bound=_Numeric)
+
+
 class CrossSiteRequestForgery(Resource):
     """
     Cross site request forgery detected.  Request aborted.
@@ -273,9 +276,9 @@ class Field:
     @classmethod
     def number(
         cls,
-        minimum: Optional[_Numeric] = None,
-        maximum: Optional[_Numeric] = None,
-        kind: Callable[[str], _Numeric] = float,
+        minimum: Optional[_N] = None,
+        maximum: Optional[_N] = None,
+        kind: Callable[[str], _N] = float,
         **kw: Any,
     ) -> "Field":
         """

src/klein/_form.py:281:37: error: Incompatible default for argument "kind" (default has type "type[float]", argument has type
"Callable[[str], _N]")  [assignment]
            kind: Callable[[str], _N] = float,
                                        ^~~~~
Found 1 error in 1 file (checked 50 source files)

[glyph]

@rouge8 to try to better explain the structure I was looking for, I opened a PR against your PR: rouge8#1 . Numerics aren't generic, because a numeric has to work with itself and (in this context, at least) only itself.

The goofy verbose @overload is just due to the fact that mypy can't understand the influence of default parameters on type signatures, i.e. python/mypy#3737 .

[glyph]

Re: the coverage failure, I don't really have time to look at it now, but there should be a coverage ignore pattern you can grab for ... in protocols somewhere, in some twisted project, that should be applied to this so it's not getting dinged for not covering protocol implementations :)

[rouge8]

Ohhh, thanks for the fix! I'll add the coverage ignore tomorrow during the workday.

[rouge8]

https://github.com/twisted/twisted/blob/dabf4622f0ecdd4f0587ae103b23982c4c83bd77/.coveragerc#L21

[glyph]

OK, I guess my change ended up being the change here :)