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

Handle parsing errors #711

Merged

Conversation

jakobandersen
Copy link
Collaborator

(This depends on / includes #698)

In a few places the C++ domain in Sphinx is used to parse fragments, but potential errors are not handled. This tries to fix that.
One case is the parsing of the function parameters in doxygenfunction, so .. doxygenfunction:: f( would crash Breathe.
The second case is parsing the function parameters in the XML, I don't know how to trigger that directly, but it should be handled now.
The third case is in the hax to insert the name in template parameters (#681). For parameters typename ...Args it will trigger a parsing error (observed in the pybind11 project).

Note, the third case is only enabled when using Sphinx 4.1 which is still not released, but probably will soon'ish.

@kelson42
Copy link

Any chance this PR will be reviewed? merged? released?

aprotyas pushed a commit to aprotyas/breathe that referenced this pull request Aug 19, 2021
Note, both of these PRs are on top of the changes introduced by
jakobandersen#698.

Links to PRs for future reference:
- breathe-doc#698
- breathe-doc#711
- breathe-doc#723

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
aprotyas pushed a commit to aprotyas/breathe that referenced this pull request Aug 19, 2021
Note, both of these PRs are on top of the changes introduced by
upstream PR breathe-doc#698.

Links to PRs for future reference:
- breathe-doc#698
- breathe-doc#711
- breathe-doc#723

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
bmerry added a commit to ska-sa/spead2 that referenced this pull request Aug 26, 2021
However, don't use 4.1, as it will need a new release of breathe to work
(e.g. see breathe-doc/breathe#711).
@jakobandersen jakobandersen force-pushed the handle_parsing_errors branch 2 times, most recently from c0ea7f3 to fe8ddb7 Compare September 1, 2021 17:39
jakobandersen and others added 2 commits September 2, 2021 08:39
Co-authored-by: Bruce Merry <1963944+bmerry@users.noreply.github.com>
@jakobandersen
Copy link
Collaborator Author

@michaeljones, unless you object, I'll merge this later today or tomorrow.

Copy link
Collaborator

@michaeljones michaeljones left a comment

Choose a reason for hiding this comment

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

Look good :)

@jakobandersen jakobandersen merged commit c84ac08 into breathe-doc:master Sep 2, 2021
@jakobandersen jakobandersen deleted the handle_parsing_errors branch September 2, 2021 15:17
@0x8000-0000
Copy link

Thank you @michaeljones and @jakobandersen !

Can we have a release now, please?

@michaeljones
Copy link
Collaborator

@vermeeren is the release manager at the moment and has been a little busy. I'm not sure when he might be able to do it. It seems reasonable to release though.

@vermeeren
Copy link
Collaborator

@michaeljones @0x8000-0000 I can definitely make a new release when/if that is desired. Past months there have been some irl issues causing an unfortunate lack of activity from my end, I really do appreciate everyone's work on this (including also @jakobandersen of course).

Definitely ok to ping me when it's time, perhaps after #735 is merged to avoid conflicts? I'll at the minimum finalise the changelog and make a formal release, not yet sure when personal issues are resolved fully after which I can get back to more proper maintainership. Thanks!

@michaeljones
Copy link
Collaborator

Thank you for the reply @vermeeren and for sharing about your current situation. I hope you're doing ok and that the personal issues are manageable and go as well as can be.

I'm happy to merge #735. It is a bit opinionated and I feel a bit out of place having wandered back in after such a time away but I think it has merit if only a small amount. I can merge in a day or two if no one has objections.

@jakobandersen - do you have anything else you would like to see merged for a release?

@jakobandersen
Copy link
Collaborator Author

@jakobandersen - do you have anything else you would like to see merged for a release?

I think merging #735 and then updating with whatever actually changed since the last release is the only thing I think is needed before a release. Unfortunately I haven't had a habit of adding changelog entries so I'm not sure what is missing.

@michaeljones
Copy link
Collaborator

I think this is the syntax for finding merges since the approximate time of the last release: https://github.com/michaeljones/breathe/pulls?q=is%3Apr+merged%3A%3E2021-05-05+

I'm happy to look through them and dig some stuff out.

We could add a PR template with a checklist for updating the CHANGELOG if desirable.

@michaeljones
Copy link
Collaborator

I've created PR with changelog & minor readme changes: #739

Hopefully when that is merged we'll be in a good place to release?

@hidmic
Copy link
Contributor

hidmic commented Sep 9, 2021

I'd love to know how far are we from a release.

@michaeljones
Copy link
Collaborator

@vermeeren - I think we're in a place to release now if you have the time at some point?

michaeljones pushed a commit that referenced this pull request Sep 14, 2021
Also chronologically sort by moving the entry for #721.
@vermeeren
Copy link
Collaborator

@michaeljones The job is done, apologies for delay. Did some minor changelog update and also fixup CICD quick in #741.

Thanks to all for the hard work!

@michaeljones
Copy link
Collaborator

Thank you @vermeeren. Kind of you to take the time when you've got other things going on.

@0x8000-0000
Copy link

Thank you @vermeeren and @michaeljones !

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.

Nested templates error
8 participants