Repository navigation
Update cachetools to 5.0.0 - #7455
Conversation
| # referenced exist anymore | ||
| cachetools\..*\.__(get|set|del)item__ # has extra argument | ||
| cachetools\..*TTLCache.__repr__ | ||
| # stubs omit defaulted arguments that are meant to be optimizations, not provided by user |
There was a problem hiding this comment.
Does it hurt that much to include the weird extra arguments in these signatures? I worry that if we allowlist all these functions, stubtest won't warn us if there are any other problems with any of them
There was a problem hiding this comment.
None of these allowlistings correspond with a def statement in the stubs. They are all inherited from other classes that don't have the extra arguments. So if we define something incorrectly in the stubs, we'll get a new stubtest error that points at the definition.
There was a problem hiding this comment.
And after thinking about this a bit more: Including the weird extra arguments also would mean including a new method definition for each of the lines I allowlisted. That would clutter the stubs and not help users in any way. CI checks should catch problems, not force you to do something weird and unnecessary IMO.
There was a problem hiding this comment.
Okay, sounds good! Will try to do a full review of the rest of the PR soon.
There was a problem hiding this comment.
Side point: I'm amazed a library that's concerned with such micro-optimisations is also using functools.total_ordering in the same module.
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
LGTM, other than the point I've raised about the TODO comment.
No description provided.