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

Address a peculiarity in Doxygen ALIASES in docu #770

Merged
merged 1 commit into from
Feb 13, 2022
Merged

Address a peculiarity in Doxygen ALIASES in docu #770

merged 1 commit into from
Feb 13, 2022

Conversation

arwedus
Copy link
Contributor

@arwedus arwedus commented Nov 28, 2021

fixes #768

@2bndy5
Copy link
Contributor

2bndy5 commented Nov 30, 2021

I've been using

ALIASES += "rst=\xmlonly<verbatim>embed:rst:leading-asterisk^^"
ALIASES += "endrst=</verbatim>/endxmlonly"

Notice I put the ^^ at the end instead of the beginning.

This has the benefit of not showing in the html output by Doxygen, and it is still compatible with breathe.

It can also be used with inline rst:

ALIASES += "inlinerst=\xmlonly<verbatim>embed:rst:inline "

@arwedus
Copy link
Contributor Author

arwedus commented Nov 30, 2021

@2bndy5 : I think that hiding the content of the rst section in Doxygen's HTML output is a side effect that not everybody wants to have. But it may be worth mentioning in this guide as well.

@2bndy5
Copy link
Contributor

2bndy5 commented Nov 30, 2021

Yeah, but it doesn't make sense to put raw rst syntax in doxygen's HTML output, especially if the rst syntax is only sought by breathe in the XML output. I believe I got the idea from exhale's docs a while back.

@arwedus
Copy link
Contributor Author

arwedus commented Jan 3, 2022

@michaeljones please merge or comment

@arwedus
Copy link
Contributor Author

arwedus commented Jan 28, 2022

@michaeljones : please review and merge.

@2bndy5
Copy link
Contributor

2bndy5 commented Jan 28, 2022

what's the rush? Docs builds are broken on RTD until #714 is addressed.

@michaeljones
Copy link
Collaborator

@arwedus I'm not a fan of your communication style. We're currently all volunteers maintaining this project and if we don't have time to move stuff forward then you can prompt in a much friendlier way than that or keep your silence or pay us to do the work you'd like us to do.

@arwedus
Copy link
Contributor Author

arwedus commented Jan 28, 2022

I actually wanted to help you out with a contribution, and I felt ignored because I did not get a review for 2 months, after being ASKED to create a PR with the fix for you. So much for communication and valuing contributions.

@arwedus
Copy link
Contributor Author

arwedus commented Jan 28, 2022

While thinking about this a bit more, and especially why I only have such unpleasant reaction here and not in other volunteer-maintained open source projects of my interest, I have a suggestion: Please, if you get a PR by a contributor who is not part of the "core team", try to respond in a timely manner, with some concrete information. E.g. "thanks, but I won't have time to review this in the next weeks/months." Or: "We will only merge this if you make the following change:". That would be polite, appreciative, and would make me a fan of your communication style. Thank you.

@arwedus arwedus closed this Jan 28, 2022
@michaeljones
Copy link
Collaborator

michaeljones commented Jan 28, 2022

Honestly, I think I can understand your perspective. I would have had similar expectations 10 years ago. (Not a comment on your age or experience, just a comment on mine.)

These days I tend to believe that you can't force interactions in an open source project. Projects exist. The maintainers have time or they don't. They prioritise your work or they don't. No contracts exist, no obligations exist. Either the current state of the project gets you closer to solving your problem or it doesn't and that is all you can rely on really.

I have grown quite distressed in the past when projects haven't met my expectations of how I thought things would go. I have realised though that it is me that needs to change not the project.

That said, I'm sorry to have caused you distress. I do appreciate you contributing. I hope you have better experiences in the future.

@2bndy5
Copy link
Contributor

2bndy5 commented Jan 28, 2022

michaeljones please merge or comment

This sounds more like a demand than a request. Despite expectations, there is still a general code of conduct which you seem to loosely follow.

@vermeeren
Copy link
Collaborator

vermeeren commented Jan 30, 2022

I actually wanted to help you out with a contribution, and I felt ignored because I did not get a review for 2 months, after being ASKED to create a PR with the fix for you. So much for communication and valuing contributions.

@arwedus Specifically about this, we have had PRs in queue for like a year in the past, which I agree is very very very much not a good thing. In the end things do get merged after potential fixup or similar, which at times we do as maintainer if the original author can't work it any longer after all the time has passed. No work goes to waste!

Also as Michael said Breathe is free, everyone uses it for free and we work on it for free. Premium service is associated with a premium price, except for perfect life circumstances which I don't have.

@arwedus
Copy link
Contributor Author

arwedus commented Jan 30, 2022

@vermeeren : Even if you have virtually no time, responding with a concrete perspective on a PR, especially after being asked for feedback, would motivate me to be a contributor; projects where the maintainers don't even respond to a contribution make a "near dead" impression to me. Now you've spent time on discussing whether a sentence I started with "please..." is a request or isn't (because I did not put in enough time to find a proper wording, maybe). I wish you would have spent the time reviewing the content of the PR. Which you still can use if you want, I am totally fine with that.

Btw. breathe is a very cool extension, so thank you for that. No hard feelings.

@vermeeren
Copy link
Collaborator

@arwedus When I initially read this I also saw Brendan's comments proposing another way, while I am not familiar with any of these two methods. It wasn't clear for me which one for me was better so I planned to research it more thoroughly and give a proper reply when I did that, instead of a half-assed "idk right now, will look later".

If you ask me I would still like to review this properly and merge it, which hopefully is within a week, these type of doc improvements are always very valuable. But since the PR is closed, I'm not sure if you still wish for it to be merged. Thoughts?

(The reason I reply now is because I finally found some time this weekend to actually get things done.)

@arwedus arwedus reopened this Jan 31, 2022
@arwedus
Copy link
Contributor Author

arwedus commented Jan 31, 2022

If you want to review it I'll reopen, sure. Regarding that comment from @2bndy5 : I have a use case where hiding output from the doxygen html output is undesirable, therefore I would recommend to add the simplest possible way to include rst sections that works to the docs. Anybody with Doxygen know how can extend it to restrict rst content to XML output, but it could be added as a further example

@2bndy5
Copy link
Contributor

2bndy5 commented Jan 31, 2022

This is what exhale's docs recommend which seems similar to the proposed changes here but without the ^^.

The ^^ is actually specific to Doxygen's ALIASES feature. I've had better consistency when I put the ^^ at the end of the \rst aliased command as it ensures that the rst syntax starts on a new line, and breathe's parsing mechanics are less liable to confuse the beginning of the rst syntax with the embed:rst:*** flag.

This was supposed to be simple comment... I'm trying to say that whitespace is very important around the embed:rst:leading-*** and using Doxygen's ^^ at the end of the alias helps ensure that breathe properly parses it.

As to the \xmlonly technique, I'd be honored if it was mentioned as an advanced usage (kinda like a "trade secret"). I may have actually devised that tactic myself and forgot about it 😆 .

@2bndy5
Copy link
Contributor

2bndy5 commented Jan 31, 2022

I have a use case where hiding output from the doxygen html output is undesirable

@arwedus That's the second time you said that, so now I'm curious: What would be a practical example in which putting raw rst in the html output is desired? If you don't want to share, then that's fine too.

@arwedus
Copy link
Contributor Author

arwedus commented Feb 1, 2022

@2bndy5: We combine Doxygen for detailed design documentation with Sphinx for API and "product" documentation. We furthermore use sphinx-needs for traceability from test case (which we just add to the code as a comment) to requirement (written in sphinx-needs directives in rst files).
That's our use case.
The rst-sections may not be interesting in the "plain doxygen" output, as you can find the same content in the generated sphinx docu, but on the other hand want to keep it included and visible - otherwise the doxygen HTML output would just be incomplete.

@arwedus
Copy link
Contributor Author

arwedus commented Feb 1, 2022

put the ^^ at the end of the \rst aliased command as it ensures that the rst syntax starts on a new line, and breathe's parsing mechanics are less liable to confuse the beginning of the rst syntax with the embed:rst:*** flag.

This was supposed to be simple comment... I'm trying to say that whitespace is very important around the embed:rst:leading-*** and using Doxygen's ^^ at the end of the alias helps ensure that breathe properly parses it.

As to the \xmlonly technique, I'd be honored if it was mentioned as an advanced usage (kinda like a "trade secret"). I may have actually devised that tactic myself and forgot about it 😆 .

Actually, I did make a typo in this PR 😆 . We actually use the alias "rst=^^\verbatim embed:rst:leading-asterisk". From our experience, verbatim txt can start right after the \rst command without another pair of newlines, but I'll give it another try before updating my PR.

I can then also include the \xmlonly trick.

@2bndy5
Copy link
Contributor

2bndy5 commented Feb 1, 2022

From our experience, verbatim txt can start right after the \rst command without another pair of newlines

This might also depend on the version of doxygen being used. That's what I meant with "better consistency". Is it important to have ^^ before \verbatim? I suspect that neither HTML nor XML would care about what column the verbatim element/tag starts on.

Good catch on the typo (surprised I missed that too).

@arwedus
Copy link
Contributor Author

arwedus commented Feb 10, 2022

From our experience, verbatim txt can start right after the \rst command without another pair of newlines

This might also depend on the version of doxygen being used. That's what I meant with "better consistency". Is it important to have ^^ before \verbatim? I suspect that neither HTML nor XML would care about what column the verbatim element/tag starts on.

The initial newline avoids that you open a verbatim section on the first line of a comment, which would then go into the brief description. This breaks the doxygen output.

I have done some tests on the newline after the "embed-rst:leading-asterisk" now.
If you add it, then indeed, the following works:

  /// \rst .. note:: A dog shall bark
  /// \endrst
  virtual void make_sound()
  {
    bark();
  }

If you don't have the "ALIAS linebreak" at the end of the \rst alias, then this would not show up on the output.

However, the generated XML shows that this is still inconsistent:

<para><verbatim>embed:rst:leading-asterisk
  .. note:: A dog shall bark
*  </verbatim> </para>

See how there is no leading asterisk after the artificial linebreak? Breathe 1.32 still handles this graciously, but I guess the cleanest way is to only start on a newline after a \rst command.

However, doxygen itself apparently does not have a limitation to start content of block commands on the next line, i.e. the following fill be parsed just fine:

///  \code template <typename ValueType>
/// struct SelectorImpl<
///    ValueType,
///    asl::EnableIfT<mops_common::IsBaseOf<ValueType, interfaces::BaseType>::value>> {
///     // ...
/// };
/// \endcode

So for maximum consistency, we should add the artificial line break at the end of the alias, too, as you suggested. Unless there would be cases where breathe can't handle it?

@2bndy5
Copy link
Contributor

2bndy5 commented Feb 11, 2022

  /// \rst .. note:: A dog shall bark
  /// \endrst
  virtual void make_sound()
  {
    bark();
  }

This feels like you're using the \rst like an \inlinerst, but I understand the point of user error here. Also, your example here supports my comment about word choice ("brief description" vs "new paragraph") because you're proposal is worded with the assumption that users will only use RST syntax to document a member.

  /// \rst .. note:: A dog shall bark \endrst

This should avoid that rogue asterisk, but that may be too specific a nuance for regular/newcomer users to remember. BTW, another rather unknown convention in RST is to always put the body of text to a directive on an indented newline. But that convention is a specific convenience for the docutils parser (RST comments should use this convention as well).

So for maximum consistency, we should add the artificial line break at the end of the alias, too, as you suggested. Unless there would be cases where breathe can't handle it?

I can't imagine a case where a line break would cause a problem for breathe. All string data is parsed (by xml.minidom) similar to buffer protocol objects (bytes or bytearray) in which a line ending in an XML's string data would be a literal \n. The only unknown I can think of is: Does Doxygen support using more than 1 ^^ in a single alias?

Additionally, some RST directives are required to end with a blank line, so it might be usefull to have a ^^ in the \endrst alias as well.

@arwedus
Copy link
Contributor Author

arwedus commented Feb 11, 2022

My example is of course artificial, that's not how we use the rst blocks in code comments.
Two linebreaks in an alias are no problem for doxygen.
Good to hear that breathe does not mind a line not starting with a leading asterisk.
Then, I guess we are good to merge?
I can't think of someone who would not put the closing tag on a newline where rst requires a newline. But if you only accept the contribution with that addition, I'll add it.

@vermeeren vermeeren self-assigned this Feb 13, 2022
@vermeeren
Copy link
Collaborator

Rebased on top of master cleanly, fixed some trailing whitespace and a missing newline on EOF. (Probably vscode defaults? Thanks microsoft.)

@arwedus and everyone else thanks for further updates and changes made to this valuable doc despite the long wait, very much appreciated!

michaeljones pushed a commit that referenced this pull request Feb 13, 2022
@michaeljones michaeljones merged commit 9bd927b into breathe-doc:master Feb 13, 2022
@vermeeren vermeeren added the documentation Generated html, changelog, inline comments label Feb 13, 2022
@arwedus
Copy link
Contributor Author

arwedus commented Feb 14, 2022

Hi all, thanks for reviewing and finally merging my contribution, I'm glad to see it added now! :-)

@arwedus arwedus deleted the issue-768-doxygen-alias-docu branch February 14, 2022 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Generated html, changelog, inline comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation is wrong about Doxygen alias for "verbatim embed:rst"
4 participants