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 mkdocs documentation #78

Merged
merged 6 commits into from
Jan 31, 2024
Merged

add mkdocs documentation #78

merged 6 commits into from
Jan 31, 2024

Conversation

roman807
Copy link
Collaborator

closes #77

Adds basic mkdocs documentation, following the example of FastAPI (see: documentation, repo)

To review, run

pdm update
mkdocs serve

and check

@roman807 roman807 linked an issue Jan 29, 2024 that may be closed by this pull request
Copy link
Collaborator

@nkaenzig nkaenzig left a comment

Choose a reason for hiding this comment

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

Looks good, I'm getting this error though:

(eva-3.10) ➜  eva (77-add-mkdocs-documentation) mkdocs serve                                                                                ✭ ✱
INFO    -  Building documentation...
INFO    -  Cleaning site directory
INFO    -  The following pages exist in the docs directory, but are not included in the "nav" configuration:
             - reference.md
ERROR   -  mkdocstrings: src.eva.vision.data.datasets.vision could not be found
ERROR   -  Error reading page 'reference/vision.md':
ERROR   -  Could not collect 'src.eva.vision.data.datasets.vision'

@roman807
Copy link
Collaborator Author

roman807 commented Jan 30, 2024

Looks good, I'm getting this error though:

(eva-3.10) ➜  eva (77-add-mkdocs-documentation) mkdocs serve                                                                                ✭ ✱
INFO    -  Building documentation...
INFO    -  Cleaning site directory
INFO    -  The following pages exist in the docs directory, but are not included in the "nav" configuration:
             - reference.md
ERROR   -  mkdocstrings: src.eva.vision.data.datasets.vision could not be found
ERROR   -  Error reading page 'reference/vision.md':
ERROR   -  Could not collect 'src.eva.vision.data.datasets.vision'

the __init__.py files were missing. can you try again?

@nkaenzig
Copy link
Collaborator

@roman807 Now it works! This looks good. I briefly compared it with the fastAPI docs and noted the following:

  1. For the parameters they have tables, which I think look nicer and are more compact:
image
  1. They have these nicely formatted examples. Apparently they come directly from the docstrings. Does our setup support this?
image
  1. They don't have the expandable boxes to display the source code (do we want to have this?)
image

@roman807
Copy link
Collaborator Author

@nkaenzig re your 3 comments from above:

(1) added the parameter tables, they are not identical to fastAPI's example, but more comprehensive (columns: name, type, description, default)

(2) yes, we will have to add such examples to the docstrings

(3) i see they have expandable source code boxes as well, e.g.: https://fastapi.tiangolo.com/reference/parameters/

Copy link
Collaborator

@ioangatop ioangatop left a comment

Choose a reason for hiding this comment

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

Looks good! We can start iterating an build from this commit, as I'm not as well are super fluent of mkdocs

when we address @nkaenzig points, lets merge it!

@ioangatop ioangatop added the documentation Improvements or additions to documentation label Jan 30, 2024
@nkaenzig
Copy link
Collaborator

@roman807 Regarding 1), the last thing I'm noticing is that in our docs, the parameters table appears in the end (after listing all the properties & methods). Is it possible to move this to the beginning like this?
image

@roman807
Copy link
Collaborator Author

roman807 commented Jan 30, 2024

@roman807 Regarding 1), the last thing I'm noticing is that in our docs, the parameters table appears in the end (after listing all the properties & methods). Is it possible to move this to the beginning like this?

@nkaenzig the table appears immediately after the method (e.g. the parameters to the method from_metrics appear after the description from the docstring. I think you meant the class- parameters (init args)? Those are currently not displayed at all, because __init__ is a private function and all params belonging to private functions are filtered out.

What FastAPI does is annotating those arguments with typing_extensions.Annotated. I'd propose we add this later.

@nkaenzig
Copy link
Collaborator

@roman807 Got it, yes seems that I was confusing the init with public methods. Displaying the __init__ and what arguments it expects would be very nice, but it's fine with me to address this later, can we create an issue for it?

Copy link
Collaborator

@nkaenzig nkaenzig left a comment

Choose a reason for hiding this comment

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

LGTM

@roman807
Copy link
Collaborator Author

@roman807 Got it, yes seems that I was confusing the init with public methods. Displaying the __init__ and what arguments it expects would be very nice, but it's fine with me to address this later, can we create an issue for it?

yes i created an issue: #83

@roman807 roman807 merged commit 5135d9c into main Jan 31, 2024
2 checks passed
@roman807 roman807 deleted the 77-add-mkdocs-documentation branch January 31, 2024 07:43
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add mkdocs documentation
3 participants