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

Do not throw in parse_tag_file if the mappings is a single Entry instead of a FunctionList #59

Closed
wants to merge 1 commit into from

Conversation

sea-bass
Copy link

I ran into a build error with the https://github.com/moveit/moveit2_tutorials repo and found that the parse_tag_file() had a mappings dictionary that expected entities to map to FunctionList objects, but many of them were Entry objects that could straightforwardly be packaged up into a FunctionList instead of throwing an exception.

I have no idea if this is correct, but it fixed the issue for me!

@sea-bass sea-bass changed the title Fix parse_tag_file Do not throw in parse_tag_file if the mappings is a single Entry instead of a FunctionList Jul 31, 2024
@JasperCraeghs
Copy link
Collaborator

JasperCraeghs commented Jan 21, 2025

I reproduced the error thrown during sphinx-build:

Extension error (sphinxcontrib.doxylink.doxylink):
Handler <function setup_doxylink_roles at 0x7c0d9a0f3eb0> for event 'builder-inited' threw an exception (exception: Cannot add override to non-function 'create_deprecated_headers::DeprecatedHeader::contents')

I think the cause is that contents is the name of both an attribute and a method. The code is functional, but it is bad practice. I suggest turning the attribute and method into a property.

I am hesitant to merge your workaround because of the added complexity that doesn't make sense. It discards the attribute in favor of the method, without reporting the duplicate name. @milliams do you have a suggestion on how to handle this corner case?

EDIT: I just realized that Doxylink doesn't officially support Python code. I'll create a new PR with a similar workaround, but more maintainable.

@JasperCraeghs
Copy link
Collaborator

PR #63 resolves this issue. I tested your use case (create_deprecated_headers::DeprecatedHeader::contents) and the Doxygen documentation for the method contents() is used as target, so not the attribute. Closing this.

@sea-bass
Copy link
Author

PR #63 resolves this issue. I tested your use case (create_deprecated_headers::DeprecatedHeader::contents) and the Doxygen documentation for the method contents() is used as target, so not the attribute. Closing this.

Thank you -- confirming that it solved the issue: moveit/moveit2_tutorials#1012

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.

2 participants