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

Improve includes list #763

Merged
merged 12 commits into from
Jan 27, 2022
Merged

Improve includes list #763

merged 12 commits into from
Jan 27, 2022

Conversation

2bndy5
Copy link
Contributor

@2bndy5 2bndy5 commented Nov 17, 2021

resolves #762

This PR is in close proximity to #725, so it would be prudent to merge that first (there may be conflict here).


I also added python v3.10 & sphinx v4.2.0 to the unit tests CI. However, I found that some versions of sphinx don't work well with python v3.10, citing:

/opt/hostedtoolcache/Python/3.10.0/x64/lib/python3.10/site-packages/sphinx/util/typing.py:34: in <module>
    from types import Union as types_Union
E   ImportError: cannot import name 'Union' from 'types' (/opt/hostedtoolcache/Python/3.10.0/x64/lib/python3.10/types.py)

These regression errors seem related to sphinx-doc/sphinx#9835. It could also be related to the cache used on the GitHub runner (I rarely use that feature myself). Either way, sphinx v3.5.4, v4.0.3, v4.1.2 are ignored from unit tests using python v3.10.

Finally, I added fail-fast: false to the unit test CI workflow's strategy because it doesn't make sense to cause all other jobs in the matrix to fail if only 1 fails.

jakobandersen added a commit to jakobandersen/breathe that referenced this pull request Nov 18, 2021
@jakobandersen
Copy link
Collaborator

I have merged the CI update from this PR with a few changes (#765). Please rebase to the newest master.

@2bndy5
Copy link
Contributor Author

2bndy5 commented Nov 18, 2021

I resolved the merge conflicts and added to the changelog this PR's changes.

@jakobandersen
Copy link
Collaborator

I have merged #725 now. Can you rebase this to the newest master?
Also, do you have a minimal example for getting multiple includes?

@@ -798,6 +798,8 @@ def buildChildren(self, child_, nodeName_):
class incType(GeneratedsSuper):
subclass = None
superclass = None
# This is an empty list used to bypass render_iterable() in visit_docreftext()
para = [] # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is fairly subjective but I would lean towards removing this and adding a check to the visit_docreftext function to see if there is a para attribute on the node in question.

I'm generally in favour of this kind of approach where you try to minimise if-statements in the code but I don't think that visit_docreftext should assume the presence of para, I suspect it is a historical assumption that is being proven wrong by this change. Unless for some reason it makes sense to think of incTypes as having paragraphs.

Copy link
Contributor Author

@2bndy5 2bndy5 Nov 20, 2021

Choose a reason for hiding this comment

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

Good feedback!

Unless for some reason it makes sense to think of incTypes as having paragraphs.

I did try wrapping the incType's text value as nodes.paragraph, but this resulted in unnecessary margin/padding from the theme I was using 👎 on every entry in the list of includes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that visit_docreftext should assume the presence of para, I suspect it is a historical assumption that is being proven wrong by this change.

I'm not sure when in the past visit_docreftext() was ever used. I don't see any other direct calls to this function - possibly called via render(method.get(node.type, visit_unknown()) at some point (presently).

I added a if hasattr(node, "para") condition to visit_docreftext() and removed my hack from compoundsuper.py. It seems to work just as well.

@michaeljones
Copy link
Collaborator

Thanks for doing this. Generally a great improvement. Kind of you to work it through.

@2bndy5
Copy link
Contributor Author

2bndy5 commented Nov 20, 2021

I noticed in the Diagrams example (of docs' "Test Suite" demo from Doxygen examples), the includes list used a combination of both types of brackets (<> or "") which resulted in some included files not getting cross-referenced. So, I added a condition in visit_docInc() to cross reference any include files if a corresponding refid attribute was found. The results (from CI build artifacts):
image


As a side note, I've found that weather to use <> or "" for include statements is a common point of confusion among amateur C++ programmers (not so much for well-versed C programmers). This is why I made any IncType objects cross-referencable if Doxygen provides a refid attribute. whereas my initial appraoch was dependant on the local attribute == 'yes'.

CHANGELOG.rst Outdated Show resolved Hide resolved
@michaeljones
Copy link
Collaborator

@2bndy5 - I've just resolved the conflict in the CHANGELOG. Are you happy for me to merge this?

@2bndy5
Copy link
Contributor Author

2bndy5 commented Jan 27, 2022

Absolutely!

@michaeljones michaeljones merged commit c12c779 into breathe-doc:master Jan 27, 2022
@michaeljones
Copy link
Collaborator

Ace! We're getting there slowly :)

@2bndy5 2bndy5 deleted the improve-includes-list branch January 27, 2022 20:00
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.

file's multiple includes on 1 line
3 participants