Question: can I do anything about docstrings for attributes being picked up from last location

[rodrigogiraoserrao]

Hey there,

I have been using mkdocstrings with mkdocs and I've hit an interesting predicament.
When documenting attributes in Python code, griffe is able to pick up docstrings from within the class body and from within the dunder method __init__. I.e., in the code below, both foo and bar would be documented:

class C:
    foo = 42
    """A number I enjoy."""

    def __init__(self):
        self.bar = 73
        """Another number I enjoy."""

However, in some cases I'll set a default value in the class body and then I'll do some initialisation inside __init__ and, in that case, griffe will pick the docstring that is used last... Even if one of the cases does not include a docstring.
E.g., in the situation below foo would not have a docstring:

class C:
    foo = 42
    """A number I enjoy."""

    def __init__(self, number):
        self.foo = number

I would expect foo to be documented with the docstring “A number I enjoy.”.
However, if I move the docstring to inside __init__ or the assignment to be after the method __init__, then it'll work.
Is this a bug? Is this an intentional design decision? Can I do anything about it?

Thanks a lot for your time!

P.S. for reference, the original exploration that led to me opening this issue is Textualize/textual#1572.

[pawamoy]

Hey @rodrigogiraoserrao (and @davep), thanks for opening this issue!

It's definitely not intentional, I just didn't think about this scenario 😄

I'm OK with modifying Griffe so that it doesn't erase previous docstrings when it finds another declaration of the same class/instance attribute :)

[rodrigogiraoserrao]

Hey @pawamoy, thanks for your timely response.

I'm OK with modifying Griffe so that it doesn't erase previous docstrings when it finds another declaration of the same class/instance attribute :)

Just to be sure, my concern had to do with Griffe erasing a docstring when it later found the same attribute without a docstring, right?
If two or more docstrings are present, I would definitely expect Griffe to preserve the last/most recent one.

Also, do you mean to say that you are ok with accepting a PR (if so, can you point me in the right direction? 😅 ) or do you mean to say you'll be adding this to a TO DO list?

[pawamoy]

Right.

• if multiple docstrings: keep the last one only (current behavior I guess)
• if same attribute without docstring, don't erase previous docstring
• ideally we should combine labels: both class-attribute and instance-attribute

All this logic happens in the visitor (src/griffe/agents/visitor.py).

The bug tracker is the Todo list :D
But PRs are welcome of course! I can provide more guidance :)

[rodrigogiraoserrao]

Taking a look at src/griffe/agents/visitor.py I would venture a guess and say that in this if statement:

griffe/src/griffe/agents/visitor.py

Lines 584 to 588 in 1ff0918

	if name in parent.members:
	# assigning multiple times
	# TODO: might be better to inspect
	if isinstance(node.parent, (ast.If, ast.ExceptHandler)): # type: ignore[union-attr]
	continue # prefer "no-exception" case

We could update the docstring if it's currently None and the one in the parent.members is not None, plus grab the labels that are already there and update them.

Does this sound about right..?

[pawamoy]

Yeah that looks right 🙂

Something like:

            if name in parent.members:
                # assigning multiple times
                # TODO: might be better to inspect
                if isinstance(node.parent, (ast.If, ast.ExceptHandler)):  # type: ignore[union-attr]
                    continue  # prefer "no-exception" case

                parent.members[name].labels |= labels

                # forward previous docstring (don't erase it)
                if parent.members[name].docstring and not docstring:
                    docstring = parent.members[name].docstring

🤔

That means the value of the attribute will be the last one appearing in the source. Maybe we should only keep the instance value and not the class one when both are defined. I'd be fine with just the docstring change though, and a TODO comment about the value 👍 We need to add tests as well of course!