-
-
Notifications
You must be signed in to change notification settings - Fork 207
Support members in a Doxygen 1.9.7 group #934
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
Support members in a Doxygen 1.9.7 group #934
Conversation
I have tried your PR hoping to fix #935 as well, but instead building the documentation with
|
I can't find the cause of the error you reported. Does the error occur with Doxygen 1.9.8 as well? |
I see a different error with 1.9.8 built myself on Fedora rawhide:
|
…e into memberdef-in-groups
@D4N I fixed the issue you reported. Thank you for catching and reporting it! @yselkowitz I'm pretty sure you didn't use the changes in this PR. Doxygen 1.9.8 is similar to 1.9.7 concerning the changes that are needed to support members in a Doxygen group. This PR does not resolve #935. |
@michaeljones Can you please review this pull request? |
I appreciate you've put effort in here but unfortunately I'm not very active on this project at the moment. We're looking for the project to be better funded. Other maintainers might have time to assess this and do the necessary follow up work. |
This is so the local "breathe" folder will have _parser.py
make "block" attribute optional for "htmlonly" tag Co-authored-by: Benjamin Cabé <kartben@gmail.com>
…e into memberdef-in-groups
@Rouslan I have pulled in your commits on @michaeljones Melexis, my employer, has sponsored this project at last. Is the amount sufficient to get things moving again? We are eager to use the latest version of Sphinx. We are now stuck with d2b0ec1 + Sphinx 6.2.1 + doxygen-1.12.0. |
@JasperCraeghs |
I appreciated the funding from your employer. I didn't know how to get in touch with anyone from your company but we're grateful for it and are happy to talk about what development priorities you might have. We also have funding from GDAL but at the recommended engagement level it would only cover 2 hours a month from 2025 which seems like it wouldn't go far. Your employer's contribution doubles that and gives more meaningful room for engaging with the project. I'm trying to figure out how work on Breathe fits into my current schedule but I hope to re-engage with the project this week. |
After some more testing with this branch in my project, I figured out the issue. If I surround the source code with the markers Example with these markers:
Produces
|
The description indicates this is built on top of the performance PR (#967) so I will look at that first. |
Or is it the other way around? The commits of the performance PR start with some of these? I would welcome some clarification on which should be reviewed and merged first. |
The contents of my commits have been thoroughly reworked by Rouslan. I doubt that any of my new code has survived. I suggest pulling in the main branch of the original repo into #967 and reviewing that. The content of both PRs looks almost identical at this point. We can keep this PR to debug the problem I reported yesterday (#934 (comment)). |
@Rouslan I resolved my issue with the latest commit. Feel free to refactor it. I hope that the parser's cache is used, but I suspect that you can come up with a cleaner solution. Since Doxygen 1.9.7, the XML file of the source file contains a If the rendered output contains a duplicate, 3 warnings are produced. I wonder if this can/should be reduced/improved See this example:
Thanks to the markers |
@JasperCraeghs Actually, your solution looks fine. It's been a while since I thought about this project and I haven't completely refamiliarized myself with it, yet, but I don't see anything that needs to be refactored. And yes, the parser's cache is used. I'll look into the 3 warnings issue later, at my leisure. Before that, I'll update the parser to get rid of the warning with the latest version of Doxygen, as you pointed out in the comments of issue #967. @michaeljones Neither PR is on top or bottom. I merged the branch into mine and then pushed the combined version back into this one. Jasper recently pulled my latest changes and then made another update, making this PR the most up to date one. |
Thank you for the clarification. I have started to review this PR. I am 21/229 files in. I think that all I can really do is try to verify that it isn't doing anything malicious, verify that it runs ok and then work on resolving conflicts with main. I don't think it is reasonable to precisely review this much code. I'll try to provide some progress reporting as I go. |
I've afraid I've been slower with progress than I'd like and I've got a busy couple of weeks coming up so progress will resume in the second week of May. I've pushed a branch called I realise it can be nice to have a squash strategy for PRs but I think we merge this one with a merge commit so that all the history is preserved otherwise it is going to be too much for one commit. |
@JasperCraeghs I've resolved the duplicate warnings about duplicate declarations. It turned out to be an easy fix. I looked into how Sphinx normally handles duplicate entries. An ID is generated for each entry. Normally the ID is based on the entry but if the ID already exists, a unique generic ID is generated. It turns out the extra warnings were not from the duplicate entry itself, but from a generated "target" node using a duplicate ID. |
I've just completed my first pass on resolving all the conflicts on the |
Needed to support XML output generated by Doxygen 1.9.7, which no longer contains duplicate member definitions (
memberdef
). The XML of the source file now contains a member reference (member
) and only the XML of the group contains the member definition.I tested these changes on large, internal projects that use Doxygen groups and the
doxygenfunction
directive. I also added a regression test: 5b23a92I am not confident that all of the changes I made are strictly necessary.
Closes #923