Repository navigation
gh-89381: Fix signature for math.log and cmath.log - #101115
hauntsaninja wants to merge 2 commits into
Conversation
Sorry, something went wrong.
The optional group in math.log seems unnecessary, as far as I can tell. Once we remove this, Argument Clinic knows what to do. For cmath, we just do what math is doing, which is use c_default = NULL (so the actual runtime logic is unchanged), but mark cmath.e as the fake default value.
|
Seems to be working. I'll keep #101070 in case someone will prefer base=None defaults. Now we got >>> inspect.signature(math.log) <Signature (x, base=2.718281828459045, /)> Maybe this is slightly cryptic: 1) magic constant appear, 2) there is an illusion that the special base=math.e works internally as log(x)/log(base). BTW, shouldn't be there regression tests (e.g. for the original issue with inspect.signature(cmath.log))? Also, I think that docs for the math.log/cmath.log should be adapted. To me it was a surprise, that this requires a manual adjustment after fixing the docstring. |
Sorry, something went wrong.
Agreed on both points; I prefer the solution in #101070. |
Sorry, something went wrong.
|
@mdickinson, I can restore #101070, if you wish. But after #101123 I'm not sure that a broken signature is an issue for CPython. |
Sorry, something went wrong.
|
@skirpichev The changes in #101070 look good to me, modulo one documentation suggestion (left in a comment). If you resurrect it, I'd be happy to merge it. |
Sorry, something went wrong.
The optional group in math.log seems unnecessary, as far as I can tell. Once we remove this, Argument Clinic knows what to do.
For cmath, we just do what math is already doing: use c_default = NULL (so the actual runtime logic is unchanged), but mark cmath.e as the fake default value.