Skip to content

fix(lib): allow DocTest to discover methods from subclasses Module#1018

Merged
patrick-kidger merged 2 commits intopatrick-kidger:mainfrom
jeertmans:patch-2
May 8, 2025
Merged

fix(lib): allow DocTest to discover methods from subclasses Module#1018
patrick-kidger merged 2 commits intopatrick-kidger:mainfrom
jeertmans:patch-2

Conversation

@jeertmans
Copy link
Contributor

Closes #1016.

In the end, there were two issues: (1) DocTestFinder._from_module was returning False because the __module__ attribute was not correct, and (2) the documentation was not copied to the wrapper class so doctests could not be discovered.

My solution is inspired from what Python is doing with their own decorators, see https://github.com/python/cpython/blob/4617d68d73409e83d6ab31106d10421d44048787/Lib/functools.py#L1033-L1049

Copy link
Owner

@patrick-kidger patrick-kidger left a comment

Choose a reason for hiding this comment

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

Nice, thank you for taking the time to figure this out!

Do you have a test to be sure that we don't accidentally break this fix in the future?

Comment on lines +683 to +691
# Fixes https://github.com/patrick-kidger/equinox/issues/1016
try:
self.__module__ = self.method.__module__
except AttributeError:
pass
try:
self.__doc__ = self.method.__doc__
except AttributeError:
pass
Copy link
Owner

Choose a reason for hiding this comment

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

Could each of these be @propertys instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it should, see below, you prefer this way?

>>> class Foo:
...   def __init__(self):
...     self.bar = 1
... 
>>> class Baz:
...   @property
...   def bar(self):
...     return 1
... 
>>> hasattr(Foo(), "bar")
True
>>> hasattr(Baz(), "bar")
True
>>> 

@jeertmans
Copy link
Contributor Author

Nice, thank you for taking the time to figure this out!

Do you have a test to be sure that we don't accidentally break this fix in the future?

I'll try to implement some tests!

…odule`

Closes patrick-kidger#1016.

In the end, there were two issues: (1) `DocTestFinder._from_module` was returning `False` because the `__module__` attribute was not correct, and (2) the documentation was not copied to the wrapper class so doctests could not be discovered.

My solution is inspired from what Python is doing with their own decorators, see https://github.com/python/cpython/blob/4617d68d73409e83d6ab31106d10421d44048787/Lib/functools.py#L1033-L1049
@jeertmans
Copy link
Contributor Author

Nice, thank you for taking the time to figure this out!
Do you have a test to be sure that we don't accidentally break this fix in the future?

I'll try to implement some tests!

Done @patrick-kidger

@patrick-kidger patrick-kidger merged commit e2d87e9 into patrick-kidger:main May 8, 2025
1 check passed
@patrick-kidger
Copy link
Owner

Awesome, this looks good to me --so merged! Thank you for the fix and for the contribution :)

@jeertmans
Copy link
Contributor Author

Thanks for your quick review and your awesome package!

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.

_wrap_method breaks doctest's ability to find class methods

2 participants