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

Added multiply inherited class for PHP objects #630

Merged
merged 1 commit into from
Feb 4, 2021
Merged

Added multiply inherited class for PHP objects #630

merged 1 commit into from
Feb 4, 2021

Conversation

RobertoRoos
Copy link
Contributor

@RobertoRoos RobertoRoos commented Feb 2, 2021

Created extra classes for PHP objects, to combine Breathe's BaseObject with phpdomain's classes, much like was already done for C#. This improves on #217

I am not entirely certain how well it works now. It does work well on this tiny example: https://github.com/RobertoRoos/PHP-Breathe-Demo

It might be good to use it on a bigger project (with good documentation) to test the result.

@vermeeren vermeeren self-requested a review February 2, 2021 21:47
@vermeeren vermeeren self-assigned this Feb 2, 2021
@vermeeren vermeeren added bug Problem in existing code code Source code regression Something broke that worked in the past labels Feb 2, 2021
Copy link
Collaborator

@vermeeren vermeeren left a comment

Choose a reason for hiding this comment

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

@RobertoRoos The diff seems good to me, but the linter is complaining a bit. Can you check? Will merge and release version if all is good. Based upon location and similar code I think this is a false positive so you can just add the # type: ignore behind it. Note that this must be prefixed with a double space.

As for further testing: I personally don't know a larger project using PHP with Breathe, usage of Breathe this way is likely pretty niche considering how few issues and patches I see about it compared to the other languages. Most likely we'll see further issues and improvements, if needed, as time passes.

Copy link
Collaborator

@vermeeren vermeeren left a comment

Choose a reason for hiding this comment

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

@RobertoRoos All good, thanks again! I intend to make a release after the remaining open PR is finished, probably within a week or so. Is that ok for you?

michaeljones pushed a commit that referenced this pull request Feb 4, 2021
@michaeljones michaeljones merged commit 886a07d into breathe-doc:master Feb 4, 2021
@RobertoRoos
Copy link
Contributor Author

Yeah, that's fine. Thank you for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Problem in existing code code Source code regression Something broke that worked in the past
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breathe throws assertion error when building docs for PHP source (through Doxygen XML)
3 participants