Skip to content

Conversation

@maribu
Copy link
Member

@maribu maribu commented Apr 8, 2025

Contribution description

Not every internal macro or obvious function parameter needs to be documented, as shown by the number of workarounds in place in our code base.

Rather than adding creating workarounds, let's just not warn about undocumented stuff.

This allows us to stop maintaining a long list of exceptions in the docchecks.

Testing procedure

After all remaining issues have been solved, the CI should be green again.

Issues/PRs references

None

maribu added 2 commits April 8, 2025 13:33
Not every internal macro or obvious function parameter needs to be
documented, as shown by the number of workarounds in place in our
code base.

Rather than adding creating workarounds, let's just not warn about
undocumented stuff.
Now that undocumented stuff is allowed, let's rather fix issues than
maintain a list of exceptions.
@github-actions github-actions bot added Area: doc Area: Documentation Area: tools Area: Supplementary tools labels Apr 8, 2025
@mguetschow
Copy link
Contributor

I do think there are good reasons to require documentation in the CI, loosing this requirement will leave the responsibility of documenting code solely to the reviewers.

@mguetschow
Copy link
Contributor

Is there no middle ground between these two extremes? Some way of marking some headers as internal and thus with no documentation requirement, for example?

@crasbe
Copy link
Contributor

crasbe commented Apr 8, 2025

A middle ground might be to have Doxygen run twice, once on origin/master and one on the current branch and make the doccheck rule that no additional errors can be introduced?
But that would make the doccheck even slower (altough one reason it is so slow is that it prints MEGABYTES of warnings) 😅

Edit: we could even make a positive reward and print the how many warnings the changes fixed :D
"Congratulations, your changes reduced the Doxygen warning count by 58. That's 0.58% of all warnings!"

Right now it's a bit of a quagmire, a lot of the exceptions probably just exist to get doccheck to pass, but should be fixed. But fixing all the warnings is an insane amount of work that probably nobody will tackle.

@maribu
Copy link
Member Author

maribu commented Apr 8, 2025

Is there no middle ground between these two extremes? Some way of marking some headers as internal and thus with no documentation requirement, for example?

I think being internal and not requiring doc are separate things. E.g. the context switching code for a given architecture is clearly internal and clearly in need of good documentation.

Another example would be FOO_SENSOR_GPIO_PIN_RESET. That is clearly not internal (but needs to be configured for the driver to work), but I would be totally fine for that to not be documented in every board that has a FOO_SENSOR configured.

I doubt that a a simple rule to decide when to document and when not is going to cut it.

@carl-tud
Copy link
Contributor

carl-tud commented Apr 10, 2025

wouldn't @internal also do the job?

@carl-tud
Copy link
Contributor

we could also use @cond or @endcond -- that would at least be an explicit expression of our intention of not wanting to document the symbols inside.

@maribu
Copy link
Member Author

maribu commented Apr 10, 2025

wouldn't @internal also do the job?

That does have a different purpose. In the Doxyfile you can configure whether you want to extract internal doc or not. Sadly, (at least last time I checked) the resulting doc does not look any different if a function/macro/... was marked as @internal or not. So @internal is pretty much useless for what we want.

We could however change the Doxyfile to explicitly not document stuff that is marked as @internal. But that would require some effort, as there indeed is some doc marked that way this is intended to be documented, but considered to be suitable for in-tree use only.

@code ... @endcode sounds like a better solution than the #ifndef DOXYGEN ... #endif that we are currently using.

@carl-tud
Copy link
Contributor

wouldn't @internal also do the job?

That does have a different purpose. In the Doxyfile you can configure whether you want to extract internal doc or not. Sadly, (at least last time I checked) the resulting doc does not look any different if a function/macro/... was marked as @internal or not. So @internal is pretty much useless for what we want.

Okay, you're right, I agree. Maybe a future documentation generator would display symbols marked @internal differently.

Regarding @cond, afaik, this also removes the symbols from the output, so not only not documented (what we do want) but also not visible (sadly, what we don't want)...

@carl-tud
Copy link
Contributor

Just reiterating... what we want is

/// @beginundocumented
int undocumented_but_visible(void);
/// @endundocumented

because the other thing we already have:

#ifndef DOXYGEN
int undocumented_and_invisbible(void);
#endif

or

/// @cond
int undocumented_and_invisbible(void);
/// @endcond

@maribu
Copy link
Member Author

maribu commented Apr 10, 2025

I think we beed both. E.g. consider gpio_init(): It will now show up 10+ times in the search (each implementation + the declaration).

Doxygen cannot know that we have vendor specific implementations of the same API, so we actually should make sure that only once instance is shown in the doc.

But for e.g. DRIVER_FOO_PARAM_I2C_ADDR the best would be to have that shown in each and every board that has a FOO sensor. But ideally the documentation should be given only once in the API doc but applied to the macro definition in every board, rather than forcing every board to copy-paste the same doc.

@carl-tud
Copy link
Contributor

carl-tud commented Apr 10, 2025

we agree on what we need.
Yet, I don't think -- just as Mikolai said -- that we should drop the requirement of enforcing documentation in the CI. imo, this would lead to less documentation overall. some symbols are not clear in contrast to people PR'ing the thing who might be mistakenly assuming a symbol's meaning and usage is well-known. there must be a middle way.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

I don't like this.... With this I will forget to document stuff, I will forget to pay attention for undocumented code in reviews. So, NACK from my side.

(That's not to say if I can be convinced if one can guarantee me that I won't forget ;-) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: doc Area: Documentation Area: tools Area: Supplementary tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants