FPGA-Research-Manchester / FABulous

Fabric generator and CAD tools
https://fabulous.readthedocs.io/en/latest/
Apache License 2.0
148 stars 34 forks source link

New Logger , Updated Docstrings & New Sphinx theme. #197

Closed A-Kibats closed 4 months ago

A-Kibats commented 5 months ago

This pull request focuses on expanding the current docstring, converting existing ones to the NumPy format and changing the Sphinx configuration to accept the format as well as updating the theme to "PyData".

Additionally the current logging system has been changed to use Loguru, files missing logging have been also updated to include it.

None of the core functionality has been changed this is more a fresh coat of paint.

All feedback is welcome especially if something has been missed.

Note

  1. Not all functions & classes have docstrings added.
  2. The formatting for the logs requires feedback to get it's final form.
  3. Docstrings may need adjusting to look good through HTML.

PyData URL

https://pydata-sphinx-theme.readthedocs.io/en/stable/index.html

Loguru URL

https://github.com/Delgan/loguru

EverythingElseWasAlreadyTaken commented 5 months ago

Thanks for the PR!

It seems that you added that one commit from master to this one, could you try to remove it ?

Also could you try to squash your commits togeter to one or two commits, that are self conatined and have a commit messaged that desctibes whats happening in the commit.

We usually use a diffent style for the docstring of our python functions, could you please convert it to our style?

A-Kibats commented 5 months ago

Hi,

Thanks for the feedback!

I'll try fix this when I've finished doing the documentation for the FABulous.py script and then squash all the commits together.

KelvinChung2000 commented 5 months ago

By the way, we might want to decide to stick to one format of doc string. When I first wrote it, I think it was based on Google's style. If you now check the reference page, the rendering with the auto doc string looks awful. I think we might want to use Sphinx + markdown to aid auto-doc generation since VSCode can show markdown content. Some references here.

EverythingElseWasAlreadyTaken commented 5 months ago

By the way, we might want to decide to stick to one format of doc string. When I first wrote it, I think it was based on Google's style. If you now check the reference page, the rendering with the auto doc string looks awful. I think we might want to use Sphinx + markdown to aid auto-doc generation since VSCode can show markdown content. Some references here.

There is also a sphinx extension for Google style docstrings: https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html

KelvinChung2000 commented 5 months ago

The current doc is already using Napoleon, and it is still looking awful. I am not sure which toggle is missing to make it look good...

After some thinking, I am thinking of going with the Numpy format now because Vscode can understand Markdown, and Numpy uses markdown elements, which make them look better in Vscode. If Sphnix can now understand Markdown, we might be able to just not use the extension and do everything with the Numpy format.

EverythingElseWasAlreadyTaken commented 5 months ago

The current doc is already using Napoleon, and it is still looking awful. I am not sure which toggle is missing to make it look good...

After some thinking, I am thinking of going with the Numpy format now because Vscode can understand Markdown, and Numpy uses markdown elements, which make them look better in Vscode. If Sphnix can now understand Markdown, we might be able to just not use the extension and do everything with the Numpy format.

I haven't really checked it in docs, but napoleon is defined in the sphinx config.py but it's not added in the docs requirements.txt. So it might never be installed and used.

Update: I'm sorry, my information was wrong. In the newer Sphinx versions, napoleon is already included. Source: https://github.com/sphinx-contrib/napoleon

KelvinChung2000 commented 5 months ago

We have concluded that the theme is causing the problem. We will update the theme. Moving to a numpy docstring should also happen, as it makes the doc string in vscode look better. Will update this pull request later.

IAmMarcelJung commented 4 months ago

Since @KelvinChung2000 requested me to review this but I currently don't have the time for a proper review I will just leave a few comments here:

First of all, thanks for your PR! We appreciate the effort you put into this.

I see the ability to use a dark theme in the doc as a big puls. We would however still have to adjust the figures, so they also fit in nicely with the dark theme.

What I personally don't like is that the chapter navigation moved to the top and does not list all chapters directly, and hides some under more. Since nowadays a aspect ratio of 16:9 or wider is most common, there is also much space at the side of the content and the navigation would fit in better there IMHO.

The FABulous logo now is way too small. I don't know if I did something wrong on my installation or if this is caused by the theme. Can you please check how it looks at your end?

image The old size of the logo.

WhatsApp Image 2024-07-03 at 23 29 58 The size of the logo after buliding locally with the new theme.

When I was building the documentation locally and created my venv just from inside the docs directory, the modules loguru and docker where still missing. Should they be added to requirements.txt or is it assumed that the venv is built in the root directory, where both modules are included in the `requirements.txt'?

A more general thing, which does not directly apply to this PR, since it was like this before already: The code documentation is under the chapter/title References. From the title I would expect there to be refences to other works, etc. but not code documentation. Is it common practice to name it like this? If so, all good and I learned something! If not, we should think about renaming it.

KelvinChung2000 commented 4 months ago

Thanks for giving it a look.

The logo will need adjustment. If the file is an SVG, it should be a scaling issue.

It is common for a code reference to be named Reference or API Reference, taking networkx as an example.

A lot of the doc I have done is trying to copy what Networkx did since they have one of the best docs I have seen...

IAmMarcelJung commented 4 months ago

Nice, then I learned something today! 😎

I agree that the Networkx doc looks really nice and clean. There it also does not disturb me that much that the chapters are on top of the page. Maybe we can get closer to that? Unfortunately I'm picky about layouts, but of no help when it comes to creating them...

A-Kibats commented 4 months ago

It's been brought to my attention that this commit has pulled changes from some of the commits in main, I'm closing this pull request and opening a new one from a clean branch that only contains the changes I've made.