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

Add merge param according to Issue #2 #3

Merged
merged 7 commits into from
Oct 25, 2024

Conversation

thomasmarwitz
Copy link
Contributor

@thomasmarwitz thomasmarwitz commented Oct 18, 2024

See #2 on context.

@thomasmarwitz
Copy link
Contributor Author

@pawamoy Sadly, I wasn't exactly able to reproduce the "Preview" feature as I found (and loved) it in your other documentations. I think I might have missed a setting. Currently looks like this:

image

@thomasmarwitz thomasmarwitz marked this pull request as ready for review October 18, 2024 09:18
@thomasmarwitz thomasmarwitz changed the title Add merge param according to Issue #1 Add merge param according to Issue #2 Oct 18, 2024
Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I find all this a bit too complex for what we need, would you allow me pushing some simplifications directly to your branch?

@thomasmarwitz
Copy link
Contributor Author

I invited you as collaborator to my fork, feel free to make some adjustments :)

@pawamoy
Copy link
Member

pawamoy commented Oct 18, 2024

Ah, thanks, actually I can just checkout this PR locally, and GitHub will allow me to push to your branch through it 😄 No need to add me as a collaborator 🙂 Sorry my previous message sounded like I was requesting some access. I just wanted to get your blessing before pushing 🙏

@pawamoy
Copy link
Member

pawamoy commented Oct 20, 2024

OK I've pushed my rather drastic change 😅

Here's how it works now: when reaching a class, we iterate on its MRO in top-down order to update its members docstrings. Going down, we know that the class above in the MRO has already inherited (and optionally merged) its docstring with its own parent, so we don't have to climb up the MRO. We just continue descending the MRO, only inheriting from the direct parent each time. We do use a set to avoid infinite loops, or even just running the same thing multiple times. It's similar to your merge cache, but we just store handled paths instead of computed docstrings, which are instead already assigned to members 🙂

What do you think 🙂?

@pawamoy
Copy link
Member

pawamoy commented Oct 20, 2024

Oh, we probably need to pass the initial set from the on_package_loaded hook to make it work. I'll push a fixup.

@pawamoy
Copy link
Member

pawamoy commented Oct 20, 2024

Done.

You'll also notice I dropped the configuration page, as it was a bit overpowered for a single option 🙂

@thomasmarwitz
Copy link
Contributor Author

LGTM

Interesting to see what changes you made, coming from a more experienced background in maintaining OSS.

@thomasmarwitz
Copy link
Contributor Author

@pawamoy, could you also create a new release once this is merged? That would be super awesome!

@pawamoy pawamoy merged commit 232fbb0 into mkdocstrings:main Oct 25, 2024
25 checks passed
@pawamoy
Copy link
Member

pawamoy commented Oct 25, 2024

Released v1.1.0 🙂

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

Successfully merging this pull request may close these issues.

2 participants