FIX: classmethods and staticmethods in profiler#59
Conversation
klauer
left a comment
klauer
left a comment
There was a problem hiding this comment.
Testing looks solid - I really like the approach you took, going so far as to check that individual functions get profiled. 👍
| output_now: bool = True, | ||
| min_threshold: float = 0, | ||
| ) -> LineProfiler: | ||
| ) -> Iterator[LineProfiler]: |
There was a problem hiding this comment.
There was a problem hiding this comment.
Haha every time I do this there's some more precise way to hint things, I'll look at this again
There was a problem hiding this comment.
The docs for this one are a whirlwind:
class typing.ContextManager(Generic[T_co])
A generic version of [contextlib.AbstractContextManager](https://docs.python.org/3/library/contextlib.html#contextlib.AbstractContextManager).
New in version 3.5.4.
New in version 3.6.0.
Deprecated since version 3.9: [contextlib.AbstractContextManager](https://docs.python.org/3/library/contextlib.html#contextlib.AbstractContextManager) now supports []
I think the cleanest thing to do is to use the 3.9 version and make sure the annotations future import is on
There was a problem hiding this comment.
Backtracking: I think my existing setup is more correct than ContextManager. I think ContextManager is for saying "this function returns a context manager that yields a LineProfiler", while I'm trying to say "this context manager yields a LineProfiler".
I'm asserting this based on how much less helpful VSCode started being with the output once I swapped it over, claiming that my "profiler" instance in the tests was type context manager.
There was a problem hiding this comment.
Oh... oh my, what a mess - I should have started at the 3 docs instead of the 3.8 docs. I'm glad you double-checked.
Good call on the annotations future approach - that is clean to me 👍
GitHub didn't show me your latest comment while I typed the above - I see now and I also learned something. Thanks!
tangkong
left a comment
tangkong
left a comment
There was a problem hiding this comment.
LGTM. I'm learning a lot about python today
3 participants
Description
Adjust the profile discovery mechanism so that it no longer skips classmethods and staticmethods. Both changes (getmembers, ismember) are needed.
Replicate the typhos PR mostly (pcdshub/typhos#505), adding tests
Motivation and Context
closes #57
closes #58
How Has This Been Tested?
Added tests, confirmed interactively that this reveals the pcdsdevices methods I was trying to see
Where Has This Been Documented?
Later in the release notes