Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get source_file of wrapped functions #657

Merged
merged 6 commits into from
Jan 17, 2024
Merged

Conversation

tmeyier
Copy link
Contributor

@tmeyier tmeyier commented Dec 25, 2023

I added a function get_source_file to doc_ast.py that tries to get the source file from obj.wrapped if inspect.getsourcefile(obj) doesn't succeed.
I used the same cached approach as in doc_ast.get_source.

closes #656

@tmeyier
Copy link
Contributor Author

tmeyier commented Dec 25, 2023

As an alternative, I thought about unwrapping cached functions in Function.__init__. But I wasn't sure about possible side effects and I found it a bit creepy to test for a cached function with

if isinstance(func, functools._lru_cache_wrapper):

@tmeyier
Copy link
Contributor Author

tmeyier commented Dec 25, 2023

I haven't implemented test coverage for the two new methods in doc_ast yet.

What kind of test would you like to have?

  • unit tests inside test/test_doc_ast.py
  • an integration test with a custom template using the function source_file (I have one that gives the source_file in the caption of the source code box)?

@mhils
Copy link
Member

mhils commented Dec 26, 2023

As an alternative, I thought about unwrapping cached functions in Function.__init__. But I wasn't sure about possible side effects and I found it a bit creepy to test for a cached function with

if isinstance(func, functools._lru_cache_wrapper):

Does that fix source extraction? That seems like a reasonable approach here. We heavily depend on Python internals anyhow. If we're concerned about this being an internal API, we can also try the import in _compat and fall back to a placeholder class.

@tmeyier
Copy link
Contributor Author

tmeyier commented Dec 26, 2023

I removed the cached version of get_source_file from doc_ast.py and cached functions are unwrapped like this, now:

        elif hasattr(func, "__wrapped__"):
            unwrapped = func.__wrapped__

According to the source code of functools, wrapped is part of the public API of functools.lrucache, so that should be reasonably safe and every function decorator using @wraps should be covered.

@mhils
Copy link
Member

mhils commented Jan 17, 2024

Thanks, this looks great now. Can we get a test + changelog entry for this? :)

Copy link
Member

@mhils mhils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the super quick turnaround! 🍰

@mhils mhils merged commit 8750631 into mitmproxy:main Jan 17, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

source_file not extracted for cached function
2 participants