Uh oh!
There was an error while loading. Please reload this page.
gh-100129: Make the names of all classes in the types module resolvable - #100130
gh-100129: Make the names of all classes in the types module resolvable#100130serhiy-storchaka wants to merge 11 commits into
Conversation
JelleZijlstra
left a comment
There was a problem hiding this comment.
I like how this change makes the builtin types more consistent and easy to resolve. However, I have two concerns:
- When the new type names appear in error messages, they make the errors harder to understand for new users. This is especially important with core types like module, function, and None.
- We'll surely break some external tools that rely on the names of these classes.
I think the case for changing is strongest for the various descriptor types, where it's otherwise difficult right now to map a type to the entry in the types module, and weakest for core builtins like module, function, and NoneType.
Uh oh!
There was an error while loading. Please reload this page.
AlexWaygood
commented
Dec 20, 2022
Agreed. I'm broadly supportive of this change, but the change to |
gvanrossum
left a comment
There was a problem hiding this comment.
I have a few nits -- feel free to address or ignore those.
I'd like @JelleZijlstra or another typeshed/mypy dev to have a quick look here to see whether changing a few names like function, ellipsis would cause any issues for static type checkers (though I doubt it).
| >>> mock = MagicMock(async_func) | ||
| >>> mock | ||
| <MagicMock spec='function' id='...'> | ||
| <MagicMock spec='FunctionType' id='...'> |
There was a problem hiding this comment.
Because the MagicMock repr contains only spec_class.__name__, not the fully qualified name.
| import sys | ||
| with self.assertRaisesRegex(TypeError, | ||
| 'Object of type module is not JSON serializable'): | ||
| 'Object of type ModuleType is not JSON serializable'): |
There was a problem hiding this comment.
Why is this not types.ModuleType?
There was a problem hiding this comment.
The same as above. It outputs type(obj).__name__.
| PyObject *obj = NULL; | ||
| if (!_PyArg_NoKeywords("cell", kwargs)) { | ||
| if (!_PyArg_NoKeywords("CellType", kwargs)) { |
There was a problem hiding this comment.
It's debatable whether this should have the types. prefix -- what do we do in other similar situations?
There was a problem hiding this comment.
Only short name is used in almost all of other similar situations (over 50 uses of this function, around 1700 uses of all PyArg_* functions). The only known exceptions are array.array, sqlite3.Connection, type.__new__, and deque.rotate.
JelleZijlstra
commented
Dec 21, 2022
Fully static type checkers like mypy never look at the runtime type objects so it shouldn't matter. Tools like stubtest or my pyanalyze typechecker might need to adapt a little but that's pretty easy. |
gvanrossum
commented
Dec 21, 2022
I was thinking about it slightly differently. Typeshed defines classes deff():
passfunc: function=fThere was some precedent for this when importtypesfunc: types.FunctionType=fwhich currently produces an error in mypy. (In a sense, this PR finally resolves a problem that mypy experienced from its creation: what is the name of the type of a function object. Grepping shows dozens of occurrences of I agree that we shouldn't stop this PR because it exposes mypy's "lie". :-) |
gvanrossum
commented
Dec 21, 2022
Thanks for the answers, go ahead and merge. |
serhiy-storchaka
commented
Dec 21, 2022
What do you think about omitting the |
markshannon
commented
Dec 21, 2022
This is going to break things. Changing the names There are a lot of changes to the tests. If this change breaks our tests, it is reasonable to assume it will break other people's tests. |
gvanrossum
commented
Dec 21, 2022
I'm happy to retract my approval in favor of more discussion -- maybe we should do that on Discourse though, where we can get the view of more people whose code actually will break. I agree that the amount of change needed in our own tests doesn't bode well. |
tungol
commented
Nov 11, 2024
Just to note: the new-in-3.13 |
This PR is stale because it has been open for 30 days with no activity. |
Documentation build overview
|
Uh oh!
There was an error while loading. Please reload this page.