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

feature: Add additional "merge" strategy / mode and allow specifying it during init #2

Open
thomasmarwitz opened this issue Oct 16, 2024 · 5 comments
Assignees
Labels
feature New feature or request

Comments

@thomasmarwitz
Copy link

Cf. mkdocstrings/python#194

Sphinx performs some merging of docstrings1, as demonstrated below.
Example:

class Shape:
    contour: list[Point]
    def surface_area(self):
        """Return the surface area in square meters."""
        return numerical_integration(self.contour)
        
class Rectange(Shape)
    def surface_area(self):
		"""Note: This is way faster than the calculation for general shapes!"""
        return distance(self.cotour[2], self.contour[0]) * distance(self.contour[3], self.contour[1])

For this example, the merged docstring for surface_area should be: "Return the surface area in square meters. Note: This is way faster than the calculation for general shapes!"

Having this feature and being able to set it during init of InheritDocstringsExtension would allow to use it in mkdocs.yml like this:

- mkdocstrings:
      handlers:
        python:
          paths: [src]
          options:
            extensions:
              - griffe_inherited_docstrings:
                  inherit_docstring_strategy: "merge"

Happy to contribute a solution :)

Footnotes

  1. Compare MetaLearner.evaluate with e.g. RLearner.evaluate. Just before Parameters, RLearner has an additional paragraph that stems from this docstring

@pawamoy
Copy link
Member

pawamoy commented Oct 16, 2024

Thanks for opening the issue here again!

So, I'm not convinced by inherit_docstring_strategy: "merge". If we have only two strategies, merge and not merge, we could just have a boolean setting: merge: true.

But then... how do we merge? In the previous issue/PR, you also had a delimiter setting IIRC, and had set it to \n\n by default. Did that mean parent docstring were truncated at the first delimiter, and then we appended the child docstring? Or did it mean something else?

Could you elaborate your use-case regarding the different ways we can merge, and give examples of parent/child classes and the resulting merged docstrings? 🙂

@thomasmarwitz
Copy link
Author

Yes, I'll elaborate more!

I thought of using inherit_docstring_strategy: Literal["if_not_present" | "merge"] = "if_not_present". S.t. users don't run another strategy by default after upgrading the package.

You're right, a merge delimiter should also be part of the config, with a default of perhaps "\n\n". A potential issue might be that some docstrings end like this:

"""blah
blah""""

and others like this:

"""blah
blah
""""

Having just one "\n" as merge delimiter would only lead to the merged child docstring becoming a new paragraph in the second case. Having "\n\n" would mitigate that as two empty lines between paragraphs are still rendered as one emtpy line in mkdocs. Merging, as I thought of it would just be MERGE_DELIMITER.join(docstrings).

So the config could look like this:

- mkdocstrings:
      handlers:
        python:
          paths: [src]
          options:
            extensions:
              - griffe_inherited_docstrings:
                  inherit_docstring_strategy: "merge" (default is "if_not_present")
                  merge_delimiter: "\n\n"

Example: merge_delimiter = "+", => denotes after merging

class Base(Obj):
    "base"    =>    "base"
    ...

class Main(Base):
    "main"    =>    "base+main"
    ...

class Sub(Main):
     "sub"    =>    "base+main+sub"
    ...

@pawamoy
Copy link
Member

pawamoy commented Oct 17, 2024

Ooooh OK I see, thanks, that's much simpler than I thought. Then I suggest we go with this:

  • a new merge boolean setting
  • always concatenate parent and child docstrings with "\n\n"

There are very low chances we change the default strategy IMO. And concatenating with "\n\n" is the most sensible thing to do. I don't think we need to allow user-customization. We can always revise all this later given user requests.

WDYT?

@thomasmarwitz
Copy link
Author

thomasmarwitz commented Oct 17, 2024

Sounds good.

One last thing to perhaps clarify: Currently, for all members docstring inheritance is performed.

Do you think we should allow customizablity like: inherit_docstring_classes: True (and for functions, attrs, ...)?

Or just wait for user requests as well?

@pawamoy
Copy link
Member

pawamoy commented Oct 17, 2024

We wait 🙂 Users come up with all kind of requests that we can't predict in advance 😄

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

No branches or pull requests

2 participants