technical-policies icon indicating copy to clipboard operation
technical-policies copied to clipboard

Add Doxygen comment requirement to coding style

Open nhorman opened this issue 1 year ago • 6 comments

In an effort to better document our code in such a way that is condusive to future developers getting involved with our code base, I thought I would propose adding a requirement to our coding style that structures/macros/functions/etc should have Doxygen style comments added to them, so that we could build alternate docs directly from our source base.

nhorman avatar May 20 '24 23:05 nhorman

@paulidale I think optional part here is up for some debate. I don't think we should mandate that everything get a doxygen comment immediately, but I think it would be good to push for documentation going forward to any function that is added or altered, so as to build up a critical mass for the use of this tool.

nhorman avatar May 21 '24 14:05 nhorman

Actually for doc-within-the-sources, isn't it much more useful for the developers of OpenSSL than for the users? Or in other words, why is Doxygen better suited for developers of apps using OpenSSL than the current manual page format?

From this it would make much more sense to me to document internals with Doxygen and keep the current .pod files format for the public API documentation.

If this is really intended to be used for public API documentation, what is the plan for converting the existing .pod files? Does it make sense to have public API documentation in two formats with possibly duplicate information in them?

t8m avatar May 22 '24 10:05 t8m

@t8m my intent was for this to be useful to future developers of openssl, not end users or developers of apps using openssl. My observation has been that one of the barriers to entry in working on openssl.is that, while the public apis are very well documented, the internals are left in a state in which "the code is the documentation" which is difficult given the complexity of our internals and creates s high bar for a new developer to get started. This change was meant to propose a method by which we could slowly start more explicitly documenting how our internal functions work to ease that burden.

nhorman avatar May 22 '24 12:05 nhorman

@t8m my intent was for this to be useful to future developers of openssl, not end users or developers of apps using openssl. My observation has been that one of the barriers to entry in working on openssl.is that, while the public apis are very well documented, the internals are left in a state in which "the code is the documentation" which is difficult given the complexity of our internals and creates s high bar for a new developer to get started. This change was meant to propose a method by which we could slowly start more explicitly documenting how our internal functions work to ease that burden.

OK, so this is for documenting internals. That makes much sense to me. Could you please perhaps adjust the wording to make it more clear?

t8m avatar May 22 '24 14:05 t8m

gladly..done

nhorman avatar May 22 '24 14:05 nhorman

Isn't it already our policy to add/update documentation, including internal?

The only change would be that if it's written as comment, we now specify a format.

kroeckx avatar May 23 '24 00:05 kroeckx

@kroeckx yes, you could make that argument. The documentation policy is located here separate from the coding style policy, and we seem to follow it fairly well for public symbols (ostensibly because we have a ci job that checks those), but we don't follow it for internal symbols very well. This is an attempt to:

  1. Make the documentation requirement more visible
  2. define a format for it to make it more generally useful for future developers getting started in our code base

nhorman avatar May 23 '24 12:05 nhorman

This is ready for vote now.

t8m avatar Jun 18 '24 09:06 t8m

Vote: 0

t8m avatar Jun 18 '24 09:06 t8m

Vote: 0

levitte avatar Jun 18 '24 10:06 levitte

Vote: +1

mattcaswell avatar Jun 18 '24 11:06 mattcaswell

-1

paulidale avatar Jun 18 '24 11:06 paulidale

Just a suggestion on this. My team sets a specific form for doxygen with specific elements so that the API documentation of our code can be derived from doxygen. This allows us to keep the API documentation near the code and in the same file - it seems to allows us to more easily keep the documentation consistent with the code.

rsbeckerca avatar Jun 18 '24 13:06 rsbeckerca

@kroeckx @romen @t-j-h @slontis @beldmit please vote

t8m avatar Jun 19 '24 17:06 t8m

Vote: +1

But I would select the '@' rather than '\' indicator as it is much more "readable" and common.

t-j-h avatar Jun 25 '24 07:06 t-j-h

Vote: 0

beldmit avatar Jun 25 '24 07:06 beldmit

Vote: +1 I also prefer @ notation.

slontis avatar Jun 25 '24 08:06 slontis

Still missing votes from @kroeckx and @romen

t8m avatar Jun 25 '24 08:06 t8m

Voting +1

kroeckx avatar Jun 30 '24 04:06 kroeckx

Vote is now closed, the policy change was accepted.

t8m avatar Jul 02 '24 08:07 t8m

@nhorman please open a new pull request with the 8061a09 commit. This needs to be separately reviewed as minor edit.

t8m avatar Jul 02 '24 08:07 t8m

style fix available in https://github.com/openssl/technical-policies/pull/98

nhorman avatar Jul 03 '24 14:07 nhorman