openssl / technical-policies

Mirror of the repository for technical policies, governed by the OTC (OpenSSL Technical Committee)
20 stars 32 forks source link

Add Doxygen comment requirement to coding style #94

Closed nhorman closed 2 months ago

nhorman commented 3 months ago

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 commented 3 months ago

@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.

t8m commented 3 months ago

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?

nhorman commented 3 months ago

@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.

t8m commented 3 months ago

@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?

nhorman commented 3 months ago

gladly..done

kroeckx commented 3 months ago

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.

nhorman commented 3 months ago

@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

t8m commented 2 months ago

This is ready for vote now.

t8m commented 2 months ago

Vote: 0

levitte commented 2 months ago

Vote: 0

mattcaswell commented 2 months ago

Vote: +1

paulidale commented 2 months ago

-1

rsbeckerca commented 2 months ago

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.

t8m commented 2 months ago

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

t-j-h commented 2 months ago

Vote: +1

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

beldmit commented 2 months ago

Vote: 0

slontis commented 2 months ago

Vote: +1 I also prefer @ notation.

t8m commented 2 months ago

Still missing votes from @kroeckx and @romen

kroeckx commented 2 months ago

Voting +1

t8m commented 2 months ago

Vote is now closed, the policy change was accepted.

t8m commented 2 months ago

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

nhorman commented 2 months ago

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