Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation Inheritance #27

Open
c-parsons opened this issue Jun 13, 2019 · 16 comments
Open

Documentation Inheritance #27

c-parsons opened this issue Jun 13, 2019 · 16 comments

Comments

@c-parsons
Copy link
Collaborator

As explained in bazelbuild/bazel#7977, there's a common use case for macros which serve as thin wrappers on rule invocations. In such cases, it's inconvenient and maintenance-heavy to manually mirror rule documentation as macro documentation:

def _rule_impl(ctx):
    bin_output = ctx.outputs.bin_output
    ...
    return [OutputGroupInfo(bin = depset([bin_output])]

_my_rule = rule(
    implementation = _my_rule_impl,
    doc = "My rule is the best rule",
    attrs = {
       "bin_output" : attr.output(doc = "Binary output"),
       "foo" : attr.string(doc = "Lorem ipsum dolor sit amet")
    }
)

def my_rule(name, **kwargs):
    """My rule is the best rule.

    Args:
      name: The name of the test rule.
      foo: Lorem ipsum dolor sit amet
      bin_output: Binary output. `name` + '.exe' by default.
      **kwargs: Other attributes to include
    """
    
    _my_rule(name = name,
        bin_output = name + ".exe",
        **kwargs)

There are several issues here:

  • Duplication between the root and argument documentation in the my_rule macro and the _my_rule rule
  • The Args section of the macro does not appropriately match the rule. There is no "foo" parameter of the macro, but it might be part of kwargs.
  • We need not document kwargs, as all kwargs should also be documented in _my_rule

I propose a macro metatag (for Stardoc to recognize) which effectively notes inheritance of documentation. So instead:

def my_rule(name, **kwargs):
    """@inherit(_my_rule)

   Args:
     bin_output: `name` + '.exe' by default.
    """
    
    _my_rule(name = name,
        bin_output = name + ".exe",
        **kwargs)

Note this new format will also need to be recognized by buildifier, so that buildifier does not emit warnings even if the Args section of the macro's docstring does not match the arguments of the macro. (Related to bazelbuild/buildtools#649)

@c-parsons
Copy link
Collaborator Author

This is an extension of bazelbuild/skydoc#182 and a duplicate of bazelbuild/skydoc#115 (though contains more detail than the original FR issue)

@laurentlb
Copy link
Contributor

cc @ittaiz

In your example, what happens with bin_output? It's an implementation detail, so we don't want to show up in the documentation, right?

@c-parsons
Copy link
Collaborator Author

My proposal is that it appends the additional docstring to that of the inherited rule. In other words, bin_output for the macro would have docstring:
"Binary output. name + '.exe' by default."

That said, I can see that some users might want to override the documentation of the inherited rule, so perhaps the macro's account of bin_output should also have a @inherit metatag

@laurentlb
Copy link
Contributor

Your example doesn't show it, but you could have extra args too.

Args:
  bin_output: ...
  other_arg: ...

Also, @inherit should ideally work with other macros too.

@c-parsons
Copy link
Collaborator Author

c-parsons commented Jun 14, 2019

Sure. Lets address these by using a metatag per arg as well.

def my_rule(name, otherarg, **kwargs):
    """@inherit(_my_rule)

   Args:
     bin_output: @argdoc(_my_rule.bin_output). `name` + '.exe' by default.
     otherarg: Some new argument.
    """
    
    _my_rule(name = name,
        bin_output = name + ".exe",
        **kwargs)

When one specifies @inherit at the top-level docstring, it will inherit everything, including args. When you explicitly specify an arg doc, it overrides by default -- so you need to specify @argdoc to create an appended docstring. You can thus also add new arguments.

@ittaiz
Copy link
Member

ittaiz commented Jun 15, 2019

+1, sounds like a good suggestion.
It’s a bit verbose but maybe that’s what needed for the richness of the features.

@thomasvl
Copy link
Member

Slightly aside, but how are defaults handled for rules vs. macros?

The example above uses the docstring on the macro to record the default value, but since defaults are also possible in macro definitions and rule definitions, wouldn't stardoc already pick those up? i.e. - would it end up generating something else to document the default which would conflict the the docstring the macro is adding?

So taking this:

def _rule_impl(ctx):
    ...

_my_rule = rule(
    implementation = _my_rule_impl,
    doc = "My rule is the best rule",
    attrs = {
       "foo" : attr.string(
           default = "default value",
           doc = "Lorem ipsum dolor sit amet",
       )
    }
)

def my_rule(name, foo="different_default", **kwargs):
    """@inherit(_my_rule)

   Args:
     foo: @argdoc(_my_rule.foo). more text here.
    """
    
    _my_rule(name = name,
        **kwargs)

How exactly would foo get documented on my_rule?

@c-parsons
Copy link
Collaborator Author

I assume you mean, for definition of my_rule:

def my_rule(name, foo="different_default", **kwargs):
    """@inherit(_my_rule)

   Args:
     foo: @argdoc(_my_rule.foo). more text here.
    """
    
    _my_rule(name = name,
        foo = foo,
        **kwargs)

(note the foo = foo)

I would hope the implementation for this feature would inherit the documentation via @argdoc, but would override the default value based on the default value of the macro. This would be a good test case :)

@thomasvl
Copy link
Member

So that works for when the macro has a default, but going back to your example:

def my_rule(name, **kwargs):
    """@inherit(_my_rule)

   Args:
     foo: @argdoc(_my_rule.foo). `name` + ' something' by default.
   """

   foo = kwargs.pop('foo', name + ' something')
    _my_rule(name = name,
        foo = foo,
        **kwargs)

Now my docstring has to document the default and the rule will also provide a default.

@c-parsons
Copy link
Collaborator Author

For what it's worth, in your above example, it wouldn't be appropriate to have foo under the Args section of the function docstring, as it is not an actual requires argument. It would be better to have foo as an explicit function parameter with default. In other words, I believe your example has an equivalent which more easily would relay information for documentation purposes.

However, we'd need to address the following example:

def my_rule(name, **kwargs):
    """@inherit(_my_rule)"""

    _my_rule(name = name,
        foo = "something",
        **kwargs)

Indeed, rule documentation already includes a documented "default value" if available, so we would need to take that into account when allowing for inheritance. We wouldn't want the above example to have foo as a documented attribute of my_rule, because it is overridden and unusable by callers.

In other words, one should be able to @inherit, but specify details such as this where inherited information can be overridden or removed.

Full disclosure, this is not something I likely have cycles for in Q4. Happy to accept contributions, though we'll likely need to evaluate a design before we delve right into implementation (we'd need to make sure these concerns are addressed!)

@UebelAndre
Copy link

Hey! Any updates on this? This feature would be awesome!!! 😄

@UebelAndre
Copy link

Another friendly ping 😅

Any updates here? 🙏

@brandjon
Copy link
Member

Hi @UebelAndre, see #89 and in particular Prioritization. We won't be working on new features in the immediate future.

@aiuto
Copy link
Contributor

aiuto commented Oct 22, 2021

Another upvote for this, with another motivating use case.
Let's say I have a .bzl file defining 2 rules (a_impl, b_impl) and their 2 wrappers (a, b), and I try to generate docs in a single target. No matter how I list the symbols in the rule, the order I get is rules first, then macros, So it comes out as

a_impl
b_impl
a
b

This is an ugly ordering, and only gets worse if I have more rules defined. What I would like is

a
a_impl
b
b_impl

I can work around this by generating each rule and macro separately, and then weaving them together, but yech.

@aiuto
Copy link
Contributor

aiuto commented Sep 23, 2022

FWIW I hacked up something like this in rules_pkg earlier this year.
I do a stardoc for each .bzl file that needs it, then post-process that to merge into a single file. I can say @wraps(_real_foo) in the macro foo's docstring and then reduce that to a single entry.

So this string ends up like this in the final form.

@tetromino
Copy link
Collaborator

Instead of macros inheriting rule docs, a better solution might be a new way of declaring macros which uses rule-like attribute lists; the attribute list can then be shared (with whatever modifications one might want to apply) between the underlying rule and the new-style macro wrapper - which would propagate docs for free and would not have the risk of docs going stale (unlike inheritance annotations).

CC @brandjon and @comius who are working on at least 2 different propasals in this area.

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

No branches or pull requests

8 participants