jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
200 stars 32 forks source link

C++ domain parameter names not conventionally required #112

Open 2bndy5 opened 2 years ago

2bndy5 commented 2 years ago

Its rather common to document a header that has no parameter names (only datatypes) when the parameter names only exist in the implementation file.

// in demo.h
/**
 * @param a info about `a`
 * @param b info about `b`
 */
void func(int, char);

// in demo.cpp
void func(int a, char b) { /* do something with params */ }

However, this theme expects the declaration signature to have the parameter names.

Furthermore, the warning message for this situation isn't very helpful:

/path/to/breathe/documentation/source/doxygen.rst::Parameter name 'b' does not match any of the parameters defined in the signature: []

Ideas

  1. It would be nice to have a workaround that allowed bypassing this warning because it is theme related, rather than solely an author problem.
  2. It would be nicer to show the originating signature for which the parameter name is being sought. I was able to hack this by changing https://github.com/jbms/sphinx-immaterial/blob/246a5c983708a24a0247cf5d6747c13c15331246/sphinx_immaterial/cpp_domain_fixes.py#L750-L756 to
               full_sig = (
                   param_node.parent.parent.parent.parent.parent.parent.parent.astext()
               )
               full_sig = full_sig[: full_sig.find("\n\n")]
               logger.warning(
                   "Parameter name %r does not match any of the parameters "
                   "defined in the signature:\n%s",
                   param_name,
                   full_sig,
                   location=param_node,
               )

    resulting in a warning that reads:

    /path/to/breathe/documentation/source/doxygen.rst::Parameter name 'b' does not match any of the parameters defined in the signature:
    void func(int, char);

    I'm sure there's a cleaner way of doing this.

jbms commented 2 years ago

The lack of line numbers is due to https://github.com/sphinx-doc/sphinx/pull/10249

With line numbers the user would at least have an easy route to figure out which signature the warning relates to.

To me it seems like the warning is kind of useful in this case, though --- wouldn't it be better to include the parameter names, so that they can be cross-linked? And also then the user would more easily know which parameter is being described by the @param comments.

In any case, sphinx does have the suppress_warnings option: https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-suppress_warnings

I think we would need to do something to give this warning a recognizable warning type so that it could be targeted by that option.

We could then provide an example in the documentation of how to suppress this warning.

2bndy5 commented 2 years ago

wouldn't it be better to include the parameter names, so that they can be cross-linked?

I'm not keen on forcing a feature on users that would break builds using the -W option. I was hoping we could provide a cross_link_param_descriptions option to allow disabling the feature for a more vanilla theme feel (especially for those trying to switch to this theme from another more basic sphinx theme).

I raised this issue specifically for the C++ domain because it could be considered a code style convention to omit param names in the function declarations. This example is taken from a breathe test that tells Doxygen not to resolve the param names when omitted from the declaration. So, it could be considered desirable (to omit param names) per project convention.


I didn't know about the suppress_warnings option. But, I'm hesitant to rely on it because the docs you linked to noted:

Now, this option should be considered experimental.

Although its documented changes indicate that it has been around for a while, it also shows the heavily active development for it (but mostly just to add more warning types).

jbms commented 2 years ago

Adding an option to entirely disable cross linking of parameters seems reasonable, if that is the behavior you want. I thought you wanted to just avoid the warnings in case they can't be cross-linked. Perhaps it would be best as an object_description_option so that it can be controlled per-domain and per object type.

2bndy5 commented 2 years ago

Adding an option to entirely disable cross linking of parameters seems reasonable, if that is the behavior you want

Yes. This theme would be more flexible if it can provide an experience similar to any other sphinx theme.

Perhaps it would be best as an object_description_option so that it can be controlled per-domain and per object type.

My thoughts exactly. But, I'm having trouble imagining the practicality of such an option in domains other than cpp.

2bndy5 commented 2 years ago

The lack of line numbers is due to https://github.com/sphinx-doc/sphinx/pull/10249

My problem with the warning isn't the lack of line numbers; that wouldn't actually help in the instance of using breathe. My problem is the empty list of parameters that it is looking in. It would be easier to read the warning if I knew what function it is talking about (thus the hack I posted in the OP).

jbms commented 2 years ago

Including the signatures in the warning makes sense --- the desc_signature nodes can be obtained from obj_content.parent.children[:-1] but then there is the issue of handling "api-include-path" so it would be better, I think, to just pass the list of signodes into that function.

2bndy5 commented 2 years ago

Implementing this in the C++ domain was easy, but it looks like the python domain has 2 instances where parameters are cross-linked to descriptions:

  1. https://github.com/jbms/sphinx-immaterial/blob/b66c9efe77bde316491279bd114f83d8f43b31c9/sphinx_immaterial/python_domain_fixes.py#L577 Which seems to affect most autodoc usage (specifically for py:parameters)
  2. https://github.com/jbms/sphinx-immaterial/blob/b66c9efe77bde316491279bd114f83d8f43b31c9/sphinx_immaterial/python_domain_fixes.py#L651 Which seems to affect direct py:function usage namely the optional syntax in our docs: https://github.com/jbms/sphinx-immaterial/blob/d874316382d3413ab4b78c2563754b4b225cc525/docs/additional_samples.rst#L225

EDIT: It looks like case 2 calls case 1. So, I making either py:function or py:parameter be controlled with the new cross_link_parameter_description option. This should follow suite with how the C/C++ domain works (in regard to the new option), although I think there are other things like c:*Param that are also effected (good thing we have the regex support).


I was able to get an adequate signature in the warning by using obj_content.parent.children[0].astext()

2bndy5 commented 2 years ago

See also https://github.com/jbms/sphinx-immaterial/pull/114#discussion_r911622440 for a fully diagnosed proposal.

2bndy5 commented 1 year ago

I'm trying to go forward with the proposal from https://github.com/jbms/sphinx-immaterial/pull/114#discussion_r911622440 which said:

Perhaps there should be some finer-grained options rather than just this one option to disable the generation of parameter objects entirely. E.g.:

object_description_options:

  • generate_parameter_objects: bool = True --- enables generation of parameter objects from :param xx: fields in the object description; these parameter objects can be cross-referenced (with synopsis shown depending on generate_synopsis), appear in search results, and can be shown in the TOC (depending on include_in_toc).
  • cross_link_signature_parameters: bool = True --- cross-links parameters defined in signatures to the parameter objects defined by generate_parameter_objects. Has no effect if generate_parameter_objects is False.
  • nitpick_parameter_names: bool = True --- emits a warning if a parameter documented with :param: can't be matched to a parameter defined by a signature. Has no effect if either cross_link_signature_parameters or generate_parameter_objects is False.

The following are my initial impressions that correspond the the above proposed list of new obj_desc_opts. Most of the problems I'm finding seem like a "chicken before the egg" conundrum.

  1. Maybe I'm misunderstanding the point of this proposed option...
  2. This was easy for me because I already had done some research about this in #114 (& the refactor since then was extremely helpful).
  3. For the CPP domain, nitpick_parameter_warnings can only be fetched if the precise parameter type is known. Since the warning is spawned from not finding the parameter in the signature (& precise parameter type is determined after the finding the parameter in the signature), there's no way to fetch the obj_desc_opt about the precise type of parameter.

Maybe I'm missing somethings, but points 1 & 3 may require a significant re-work of the code base. Like I said, these are just my first impressions (since letting this issue get stale after #114). TBH, I still think a single option to allow/disallow the cross-linking of parameters in signatures would be the simplest solution for this issue.

jbms commented 1 year ago

I think the chicken and egg issue can be solved by making nitpick_parameter_names and generate_parameter_objects options of the parent object (e.g. cpp:func) rather than the parameter object itself.

2bndy5 commented 1 year ago

Well the parent tactic worked for nitpick_parameter_names, but now I'm having trouble differentiating between the actual code needed to cross-link parameter fields and the creation of parameter objects from :param x: fields. Locally, I'm just treating the cross_link_signature_parameters the same as generate_parameter_objects.

Instead of altering the docs, I'm writing unit tests to ensure the options are doing what they're supposed to.