Skip to content

MDEV-39718: Produce Markdown plugin API documentation#5112

Draft
gkodinov wants to merge 1 commit into
mainfrom
mdev-39718
Draft

MDEV-39718: Produce Markdown plugin API documentation#5112
gkodinov wants to merge 1 commit into
mainfrom
mdev-39718

Conversation

@gkodinov
Copy link
Copy Markdown
Member

Implemented a cmake utility macro to generate markdown documentation for the plugin API headers.
Fixed some doxygen comment mistakes in these.

Implemented a cmake utility macro to generate markdown documentation
for the plugin API headers.
Fixed some doxygen comment mistakes in these.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new CMake utility to generate plugin API documentation using Doxygen and Moxygen, along with various documentation formatting improvements in header files to ensure proper rendering. The review identifies a critical bug in the CMake macro where the first source file is inadvertently removed from the list, causing incomplete documentation. Additionally, the feedback suggests using functions instead of macros for better encapsulation, adopting more idiomatic boolean options in CMake, and defaulting the documentation generation to off to avoid build-time dependency issues in environments lacking the required tools.

Comment on lines +39 to +40
SET(SOURCES ${ARG_UNPARSED_ARGUMENTS})
LIST(REMOVE_AT SOURCES 0)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This line removes the first element from the SOURCES list. Based on the usage in include/CMakeLists.txt, where the macro is called with a list of header files, this will cause the first header (e.g., mysql/plugin.h) to be excluded from the generated documentation. If the intention was to handle a target name as the first argument, the macro signature and the call site should be updated accordingly; otherwise, this removal should be deleted.

  SET(SOURCES ${ARG_UNPARSED_ARGUMENTS})

# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA

SET(WITH_PLUGIN_API_DOCS yes CACHE STRING "Produce the API docs")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The option WITH_PLUGIN_API_DOCS is defined as a STRING with a default of yes. It is more idiomatic in CMake to use the OPTION() command or SET(... CACHE BOOL ...) for boolean flags. Additionally, since this feature introduces new build-time dependencies (doxygen and moxygen), it should ideally default to OFF to avoid breaking existing build environments that lack these tools by default.

OPTION(WITH_PLUGIN_API_DOCS "Produce the API docs" OFF)

# Call with a list of sources, e.g.
# PLUGIN_API_DOCS_GENERATE(foo.h bar.h)

MACRO (PLUGIN_API_DOCS_GENERATE)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using FUNCTION is preferred over MACRO in CMake to prevent internal variables (like SOURCES, ARG_UNPARSED_ARGUMENTS, and the DOXYGEN_* configuration variables) from leaking into the caller's scope. Since this logic defines a build target and sets local configuration, a function provides better encapsulation.

FUNCTION (PLUGIN_API_DOCS_GENERATE)

BYPRODUCTS ${CMAKE_BINARY_DIR}/api_docs/md/api.md
COMMENT "Generating plugin API docs with moxygen"
)
ENDMACRO()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Change to ENDFUNCTION() to match the suggested change from MACRO to FUNCTION.

ENDFUNCTION()

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants