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

BUG: validator now handles decorators #496

Merged
merged 5 commits into from
Aug 30, 2023
Merged

Conversation

sdiebolt
Copy link
Contributor

@sdiebolt sdiebolt commented Aug 24, 2023

This PR fixes a bug that prevented Validator.source_file_def_line to return the correct line number of the def and class keywords when the function/class used decorators.

This issue made validate miss inline ignore comments: extract_ignore_validation_comments would return a dictionary with keys corresponding to the line numbers of def and class keywords, but it would then be indexed using Validator.source_file_def_line, which would correspond to the line of the first decorator, not the def/class keyword.

I'll try to add a regression test ASAP.

Copy link
Collaborator

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Looks good so far, let me know when the test is there and things are green and I think we can merge!

numpydoc/validate.py Outdated Show resolved Hide resolved
@sdiebolt
Copy link
Contributor Author

sdiebolt commented Aug 29, 2023

@larsoner I've written a simple regression test, but I'm unsure if this fits in the existing testing pipeline. Please don't hesitate to indicate any issue or possible improvement, I don't mind rewritting these to better fit the existing tests.

On another note, I've just noticed that a similar bug still exists for @property decorated methods. The fix proposed here does not work, as inspect.getsourcelines raises a TypeError when passed a property object, leading Validator.source_file_def_line to return None. This may be related to #486 and #494.

I think we may be able to fix this by passing the fget attribute of property objects, which is the getter function, to inspect.getsourcelines. I think numpydoc only parses the getter anyway, so that should not be a problem. Shall I fix this in this PR or open a new one?

EDIT: I've tested the proposed fix and it works correctly when fixing Validator.source_file_name also. I think this might require some new tests though.

@sdiebolt sdiebolt marked this pull request as ready for review August 29, 2023 16:50
@sdiebolt sdiebolt changed the title WIP, BUG: validator now handles decorators BUG: validator now handles decorators Aug 29, 2023
@larsoner
Copy link
Collaborator

Failure looks related, can you check?

I think we may be able to fix this by passing the fget attribute of property objects, which is the getter function, to inspect.getsourcelines. I think numpydoc only parses the getter anyway, so that should not be a problem. Shall I fix this in this PR or open a new one?

Probably better as a follow-up PR if we can merge this one quickly, which I think we can. But the current changes are small so if you think it makes sense to combine them then I can live with that, too.

@sdiebolt
Copy link
Contributor Author

sdiebolt commented Aug 30, 2023

Ok this is a weird one. Seems like inspect.getsourcelines does not catch decorators from a decorated class in Python 3.8. It does for functions though. I'm not sure why this is the case. I'll modify the tests to take it into account.

@sdiebolt
Copy link
Contributor Author

This was indeed a bug in CPython: python/cpython#60060. The fix was only implemented starting with 3.9 if I understand it correctly. Tests should be passing for 3.8 now.

@sdiebolt
Copy link
Contributor Author

Feel free to merge if everything is OK and I'll start the follow up PR. 🙂

@larsoner larsoner enabled auto-merge (squash) August 30, 2023 14:07
@larsoner
Copy link
Collaborator

Marking for merge-when-green, thanks in advance @sdiebolt !

@larsoner larsoner merged commit df55b8a into numpy:main Aug 30, 2023
23 checks passed
@stefanv stefanv added this to the 1.6.0 milestone Aug 30, 2023
@sdiebolt sdiebolt deleted the decorator-patch branch August 30, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants