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: Convert docstring to Markdown #2159

Conversation

s-martin
Copy link
Collaborator

@s-martin s-martin commented Dec 12, 2023

This PR adds an action, which creates a new PR, every time Python code changes have been merged to future3/develop.

This PR creates Markdown documentation of the Python code from docstring.

The Markdown files are written to documentation/developers/docstring.

See s-martin#28 for proof of concept.

@coveralls
Copy link

coveralls commented Dec 12, 2023

Pull Request Test Coverage Report for Build 7239165628

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-14.2%) to 38.178%

Totals Coverage Status
Change from base Build 7174343890: -14.2%
Covered Lines: 415
Relevant Lines: 1087

💛 - Coveralls

@s-martin
Copy link
Collaborator Author

s-martin commented Dec 12, 2023

For the repo in Settings - Actions - General

Screenshot_20231212-143240.png

the permission to create PRs must be enabled (only the checkbox is necessary!).

@MiczFlor

@AlvinSchiller
Copy link
Collaborator

For the repo in Settings - Actions - General

Screenshot_20231212-143240.png

the permission to create PRs must be enabled.

@MiczFlor

The other permissions should not be changed right? As all read and write permissions for actions is mostly not necessary and can also be adjusted on other ways.

@AlvinSchiller
Copy link
Collaborator

AlvinSchiller commented Dec 14, 2023

See s-martin#26 for proof of concept.

Did you mean your doc-update branch? On the s-martin#26 branch i dont see the generated docs.

@s-martin
Copy link
Collaborator Author

Correct

@s-martin
Copy link
Collaborator Author

See s-martin#26 for proof of concept.

Did you mean your doc-update branch? On the s-martin#26 branch i dont see the generated docs.

You are right, s-martin#28 is the result PR.

s-martin#26 was just me fiddling around.

@AlvinSchiller
Copy link
Collaborator

Most of the docs are empty. This should then be filled over time, right?
Otherwise there snot much use for the docs ^^

Also the Readme.md contains links to the subpages, but also all other information. It looks pretty rough and not really usable.

@s-martin
Copy link
Collaborator Author

It's empty, when there's no docstring in the Python file.

Unfortunately the action doesn't have the possibility to filter for "empty documentation" yet. Probably something to improve.

@MiczFlor
Copy link
Owner

For the repo in Settings - Actions - General

Screenshot_20231212-143240.png

the permission to create PRs must be enabled (only the checkbox is necessary!).

@MiczFlor

@s-martin
What am I missing here? It looks like this:
Screenshot from 2023-12-16 21-14-18

@s-martin
Copy link
Collaborator Author

Ok, that’s weird. Thought this was the missing setting.

is this also set? Beginning of the page.

IMG_0589

@AlvinSchiller
Copy link
Collaborator

PRs from forked repos are restricted to only have read permissions, no matter what settings are made.
I think this shall prevent malicious access if the workflow are automatically started.

So after the merge it should run fine, if not pull_request trigger is needed.
Didn't think about it earlier ...

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflows-in-forked-repositories

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository. The GITHUB_TOKEN has read-only permissions in pull requests from forked repositories. For more information, see "Automatic token authentication."

@s-martin
Copy link
Collaborator Author

s-martin commented Dec 17, 2023

Most of the docs are empty. This should then be filled over time, right? Otherwise there snot much use for the docs ^^

Also the Readme.md contains links to the subpages, but also all other information. It looks pretty rough and not really usable.

I checked the result PR in my repo s-martin#28

at least 80% of the files contain at least some doc info, so I would say in general it’s usable and I would not put effort in implementing to skip empty content, but rather focus on adding the missing info. For that we could use flake-docstring to help with identifying missing content.

To enforce docstrings with flake8 we could use something like s-martin#29 later.

the Readme here contains also the class description, which I find personally OK for an overview.

What do you think?

PS. I have a fork of the action to create the markdown docs to make it working here: https://github.com/s-martin/markdown-docs
If we decide to implement a skipping empty stuff in the action we could use that fork.

@AlvinSchiller
Copy link
Collaborator

AlvinSchiller commented Dec 17, 2023

at least 80% of the files contain at least some doc info, so I would say in general it’s usable and I would not put effort in implementing to skip empty content, but rather focus on adding the missing info. For that we could use flake-docstring to help with identifying missing content.

Building up the docs should be the way, so fine for me 👍

the Readme here contains also the class description, which I find personally OK for an overview

The class overview is ok, also it's unsorted and has duplications. But I rather meant the functions section. I can't really tell what they are about. Just an unordered list of all functions available?


Edit: ok found an answer in the Readme

title: title
classes: list of class scoped objects
functions: list of all functions scoped objects not defined in a class

Im really unsure about that. Maybe just leave it as is for now, so we can use it as a base to check later on, if all function scopes are correct or could be improved?

@AlvinSchiller
Copy link
Collaborator

AlvinSchiller commented Dec 17, 2023

PS. I have a fork of the action to create the markdown docs to make it working here: https://github.com/s-martin/markdown-docs
If we decide to implement a skipping empty stuff in the action we could use that fork.

👍
I'm not familiar enough with python, but does it make sense to also export all the internal functions (leading underscore)? I would not expect them to be documented. Should them be left out? flake8-docstrings (s-martin#29) also only warns for 'public' functions.
And is it configurable to sort the function?

@AlvinSchiller
Copy link
Collaborator

AlvinSchiller commented Dec 17, 2023

And i think we have to set our own configuration.
One Example:
Our param definition in the code is currently ':param name: description', but the configured is '@ param name description'

@s-martin
Copy link
Collaborator Author

Our param definition in the code is currently ':param name: description', but the configured is '@ param name description'

Good catch 👍

@pabera
Copy link
Collaborator

pabera commented Dec 17, 2023

Couldn't docstring not be handled as a Git pre-commit hook instead of a Github Action to PR?

@s-martin
Copy link
Collaborator Author

I don't know, but this would at least create the docs with the source code change.

@s-martin
Copy link
Collaborator Author

I don't know, but this would at least create the docs with the source code change.

This looks a little bit more mature than the GitHub Action this PR is using:
https://niklasrosenstein.github.io/pydoc-markdown/

If we use this in a pre commit hook, is it possible to stage the generated files for the same commit?

@AlvinSchiller
Copy link
Collaborator

Not a bad idea either. And sounds like less overhead.
I see two problems, but these could be resolved.

  • Githooks must be activated on the machine, so there is some sort of setup necessary (manual or automated).

  • if the hook e.g runs a python lib, python must be installed as well (I have it currently not installed).

So there is a chance that it's not run by the user, if the setup is not performed (like the current hook for flake8 ).

So maybe a solution to address this is to supply a development container? And for activation something like this?
Also like flake8, the docs could be enforced with a GitHub action like @s-martin already mentioned.

@s-martin
Copy link
Collaborator Author

So maybe a solution to address this is to supply a development container? And for activation something like this?

This was already suggested a while ago #1941

Also like flake8, the docs could be enforced with a GitHub action like @s-martin already mentioned.

@pabera
Copy link
Collaborator

pabera commented Dec 17, 2023

A few thoughts. Some of it has been said to some extend, I just wanted to summarize it.

  1. I like docstring, as it reveals a lot of info from the py files that would be hidden. But, Docstring to markdown update s-martin/RPi-Jukebox-RFID#28 doesn't make it much better. Lots of files are just boilerplate which can lead to frustration/confusion when looking for something specific. (but I don't know if that would be the same for PyDoc :-)

  2. I tried using Pydoc Markdown, but I had some trouble with it. But I would give it another go.

  3. I would NOT suggest to run the DocsToMD through an Github action which triggers a PR. I would add it to flake8 and break the build if docs have not been build accordingly, in case some people have not enabled git hooks. If they have, they will be informed anyways.

  4. if the hook e.g runs a python lib, python must be installed as well (I have it currently not installed).

    We can let it run in a Docker-Container. I tried this with Sphinx already almost worked but then it was just too complicted. I believe this should be easier with PyDoc or docstring.

@s-martin
Copy link
Collaborator Author

My main driver was to make the plugin docs hidden in plugs.py more visible.

I like the approach enforcing the docs before the commit, I’m not bound to the Action :)

I don’t know, if pydoc markdown is the most suitable tool, it was just a little googling and it seemed simple enough and did not require stuff like Sphinx etc.

Docker container sounds like an interesting idea, but can’t we expect that someone implementing stuff in Python having Python installed? 😜
At least, if we can limit the hook to py files?

Right now, the pre commit hook is a custom implementation, right? Is it worth it to use something like https://github.com/pre-commit/pre-commit-hooks, which was already suggested in #1941?

@s-martin
Copy link
Collaborator Author

Simpler solution here: #2181

Closing this one

@s-martin s-martin closed this Dec 26, 2023
@s-martin s-martin deleted the future3/feature/docstring2markdown branch December 26, 2023 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation enhancement future3 Relates to future3 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants