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

feat(docs): generate docs from docstrings #5131

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from

Conversation

JP-Ellis
Copy link

@JP-Ellis JP-Ellis commented Sep 5, 2024

Issue number: #5125

Summary

Changes

This is a fairly major change to the docs and generates documentation for all public methods, attributes and variables in AWS Powertools.

This is achieved using mkdocstrings, which replaces instances of

::: some.module.path

with the documentation of the specified module.

As a side effect of generating the documentation, an objects.inv file is also generated.

From a quick purview of the generated docs, they seem pretty good as the underlying code appears well documented; however, this probably needs to be double checked. Especially for cases where the Numpy docstring style is not being followed which may result in errors generating the docs.

User experience

There is no change to the usage of AWS Powertools.

There will be a significant change to the documentation in that the classes/functions/variables will be made public. Most editors would already be presenting this information to developers already, but this just makes it discoverable online.

The main benefit here is that other docs which want to refer to a specific class or function from AWS Powertools can now do so using the objects.inv file generated.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

  • Meet tenets criteria
  • I have performed a self-review of this change
  • Changes have been tested
    • I have tested the generation of docs locally, but I am not familiar with the rest of your pipeline for the documentation.
  • Changes are documented
    • As these are changes to the documentation itself, they should be self-evident to the end users
    • I would be happy to be pointed to most appropriate for the documentation on the documentation
  • PR title follows conventional commit semantics

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

This is a fairly major change to the docs and generates documentation for all
public methods, attributes and variables in AWS Powertools.

This is achieved using `mkdocstrings`, which replaces instances of

```markdown
::: some.module.path
```

with the documentation of the specified module.

As a side effect of generating the documentation, an `objects.inv` file is also
generated.

From a quick purview of the generated docs, they seem pretty good as the
underlying code appears well documented; however, this probably needs to be
double checked. Especially for cases where the Numpy docstring style is not
being followed which may result in errors generating the docs.

Ref: aws-powertools#5125
Signed-off-by: JP-Ellis <[email protected]>
@JP-Ellis JP-Ellis requested a review from a team as a code owner September 5, 2024 22:40
@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation internal Maintenance changes labels Sep 5, 2024
Copy link

boring-cyborg bot commented Sep 5, 2024

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 5, 2024
Copy link

sonarcloud bot commented Sep 5, 2024

@github-actions github-actions bot added feature New feature or functionality and removed documentation Improvements or additions to documentation internal Maintenance changes labels Sep 9, 2024
@leandrodamascena
Copy link
Contributor

Hey @JP-Ellis thanks for working in this PR and creating a reproduicle outcome from what we can expect.

Let me split down my feedback to make easy we agree on the next steps.

pdoc3 vs mkdocstrings

We already have an API documentation created by pdoc3, which works well. Personally, I like it; however, I see much more value in using mkdocstrings because it integrates seamlessly with our documentation and offers greater potential to utilize features from MkDocs, which is fantastic. In addition, we already have an issue opened for this migration, which is a great step forward to agree with this kind of work.

Testing this PR

2 - I tested this PR locally and I was able to create the objects.inv file and render the new API tab, which is great to see this working and how it looks like.

Current limitations/problems

3 - Even if we agreeing to migrate from pdoc3 to mkdocstrings, I see some problems in accepting this PR as it is. To move forward, we need to rethink how to do this. In this PR, you are adding 55 new files and marking some to be ignored by markdown lint, which may have some undesirable effects. Also, I admit that our codebase is far from being suitable for mkdocstrings to render this perfectly - or even good. Here are some examples:

We should not render these attributes
image

The code is completely broken here
image

We should not have these special characters in the examples
image

Links without href tag
image

Making a smooth transition

I'm thinking about making a smooth transition from pdoc3 to mkdocstrings and splitting it by utility, rather than all utilities at once. I suggest this because we'll need to change a lot in our docstrings, comments, and others to have documentation that is actually usable by customers. Otherwise, we'll just introduce a new section that doesn't have much value and may generate doubts about how to use it.

Enabling a new Ruff rule

5 - We can use this opportunity to progressively enable a new Ruff linter rule: https://docs.astral.sh/ruff/rules/#pydocstyle-d. We didn't enable that because our docstrings and comments need to be revamped.

Next steps

So, based on what I explained before, I suggest the next steps are:

1 - Modify this PR to add just one utility and make all the changes we need to make to the mkdocstrings + docstrings configuration. I suggest we start with BatchProcessing as the codebase is not complex and we can do an excellent job on it that will serve as a base for the others.
2 - Enable the Ruff D rule on this PR and see which files Ruff complains about.
3 - Publish the first version with this new change and see how this looks like.

Please let me know what you think and if you would like to make these changes and go through some back-and-forth review process. If you don't have time to do this —we know everyone is very busy— I can handle it and ping you for reviews and feedback. Please let me know your thoughts.

Again, thanks a lot for kicking out this to make our documentation even better.

@leandrodamascena leandrodamascena added the on-hold This item is on-hold and will be revisited in the future label Sep 9, 2024
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

Leaving a comment to Request Changes.

@leandrodamascena leandrodamascena linked an issue Sep 9, 2024 that may be closed by this pull request
1 task
@JP-Ellis
Copy link
Author

JP-Ellis commented Sep 9, 2024

Amazing feedback!

I should have made it clear at the start that this is meant to be a first proof-of-concept and step towards MkDocstrings. I didn't want to invest too much time in case this wasn't going to be adopted. I'm glad to hear this is something you are keen on!

To answer some of your more specific feedback.

pdoc3 vs mkdocstrings

We already have an API documentation [and] we already have an issue opened for [migrating to MkDocs]

I'll be honest, I completely missed that. But I'm glad to hear this was in the pipeline already!

In this PR, you are adding 55 new files and marking some to be ignored by markdown lint, which may have some undesirable effects.

Completely agree. When I personally use this, I actually create a small Python scripts that is invoked by Mkdocs to automatically generate the Markdown files on-the-fly. There are a couple of pros and cons:

  • Pro: The Markdown files are guaranteed to correspond to the Python files
  • Pro: No need to commit a large number of mostly-the-same Markdown files
  • Con: It may seem a bit 'magical' to someone unfamiliar
  • Con: It doesn't allow for customisation of the Markdown file on a per-file basis (I would say that for API docs, this is typically not a big issue)
  • Con: It adds yet another dependency, though only for building the docs.

Also, I admit that our codebase is far from being suitable for mkdocstrings to render this perfectly [...] we'll just introduce a new section that doesn't have much value and may generate doubts about how to use it.

Yeah, that's completely fair. A per-utility approach would certainly be possible. Since you already have the pdoc3 docs up, you could also put a disclaimer during the transition to the effect of

Note

The below docs are in the process of being transitioned and may contain some formatting issues.

We can use this opportunity to progressively enable a new Ruff linter rule: https://docs.astral.sh/ruff/rules/#pydocstyle-d. We didn't enable that because our docstrings and comments need to be revamped.

Absolutely a fantastic thing to add. Just be aware that Ruff doesn't necessarily enforce all docstring rules all the time as it tries to be smart about which lints are applicable to which rules. As a concrete example in repos I maintain, Ruff will detect if there is a mismatch between the args documented and the args in the function; but it will not raise a lint if the docstring has no args at all. This can be problematic if the args are in fact documented but in a style which Ruff did not expect.

With the pace of Ruff's development, this may have changed over the past months, but that is something to be aware of.

  1. Modify this PR to add just one utility and make all the changes we need to make to the mkdocstrings + docstrings configuration. I suggest we start with BatchProcessing as the codebase is not complex and we can do an excellent job on it that will serve as a base for the others.
  2. Enable the Ruff D rule on this PR and see which files Ruff complains about.
  3. Publish the first version with this new change and see how this looks like.

Sounds good! I won't get much time this week to contribute, but hopefully this weekend or next week I should get a bit of time :)

@leandrodamascena
Copy link
Contributor

Hey @JP-Ellis! Thanks for the feedback and clarifying things about this PoC. I am writing additional feedback based on your answers.

Completely agree. When I personally use this, I actually create a small Python scripts that is invoked by Mkdocs to automatically generate the Markdown files on-the-fly. There are a couple of pros and cons:

Yes, we should avoid doing this. My observation about the number of files is because it makes it difficult to review the PR and we can lose attention with too many files. That's why I suggested dividing this into small PRs by utility, which will make our work easier.

Yeah, that's completely fair. A per-utility approach would certainly be possible. Since you already have the pdoc3 docs up, you could also put a disclaimer during the transition to the effect of

Perfect, this is a really good idea.

Absolutely a fantastic thing to add. Just be aware that Ruff doesn't necessarily enforce all docstring rules all the time as it tries to be smart about which lints are applicable to which rules. As a concrete example in repos I maintain, Ruff will detect if there is a mismatch between the args documented and the args in the function; but it will not raise a lint if the docstring has no args at all. This can be problematic if the args are in fact documented but in a style which Ruff did not expect.

With the pace of Ruff's development, this may have changed over the past months, but that is something to be aware of.

Hmmm, I didn't know that. I'll spend some time checking this out and see what limitations/caveats we should be aware of and deal with "manually" when doing this work.

  1. Modify this PR to add just one utility and make all the changes we need to make to the mkdocstrings + docstrings configuration. I suggest we start with BatchProcessing as the codebase is not complex and we can do an excellent job on it that will serve as a base for the others.
  2. Enable the Ruff D rule on this PR and see which files Ruff complains about.
  3. Publish the first version with this new change and see how this looks like.

Sounds good! I won't get much time this week to contribute, but hopefully this weekend or next week I should get a bit of time :)

Perfect! Take your time, no rush at all and will be amazing to make this change and improve our documentation.

Thanks for taking the time to do this and let's make it happen 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality on-hold This item is on-hold and will be revisited in the future size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: Addition of docs inventory (objects.inv)
2 participants