BIMAU / transiflow

Implementations and continuation of some standard computational fluid dynamics problems using the finite volume method
https://transiflow.readthedocs.io/
Apache License 2.0
9 stars 12 forks source link

Sphinx documentation build #32

Closed LourensVeen closed 5 months ago

LourensVeen commented 5 months ago

Here is a Sphinx-based documentation build. You can build the documentation locally by creating a virtualenv, installing the dependencies in docs/requirements.txt, and then running make html in the doc subdirectory. The included .readthedocs.yml file should be a good first stab at a working RTD configuration, but you'll have to set that up on RTD and test.

I got some errors on some of the imports, where it's unclear whether you're importing a module or a class, so I fixed those, and there's no .gitignore so I added one with at least one directory type that should be ignored.

Sbte commented 5 months ago

Actually, it seems like flake8 disagrees. And the second commit seems a bit confused.

LourensVeen commented 5 months ago

Actually, it seems like flake8 disagrees. And the second commit seems a bit confused.

I'm not sure what this refers to, the second commit? Something weird definitely happened to the commit message :smile:. Probably hit the wrong key in vim.

The issue here is that you have a module named transiflow.Discretization with inside of it a class Discretization. You then import that class in transiflow/__init__.py and re-export it as transiflow.Discretization. So is transiflow.Discretization now a class or a module?

At any rate, module names should be lowercase in Python, I'm surprised flake8 isn't complaining about that but I haven't checked the configuration.

LourensVeen commented 5 months ago

Ah, I see, Sphinx generates a configuration file that isn't PEP-8 compliant. Not ideal. I've pushed a fix.

Sbte commented 5 months ago

Actually, it seems like flake8 disagrees. And the second commit seems a bit confused.

I'm not sure what this refers to, the second commit? Something weird definitely happened to the commit message 😄. Probably hit the wrong key in vim.

The flake8 warnings from the CI are about the first commit.

The issue here is that you have a module named transiflow.Discretization with inside of it a class Discretization. You then import that class in transiflow/__init__.py and re-export it as transiflow.Discretization. So is transiflow.Discretization now a class or a module?

At any rate, module names should be lowercase in Python, I'm surprised flake8 isn't complaining about that but I haven't checked the configuration.

Discretization.py is just a file that stores the Discretization class. It shouldn't be treated as a module. I assume the "Pythonic" way would be to dump everything into a single file and then you'd have classes that are not modules. I don't like this idea. If there is a better way to do this, I'd like to hear about it.

LourensVeen commented 5 months ago

A Python file is a module (namespace that you can import things from), and a directory is a package (not to be confused with the thing you download from PyPI) if it has a __init__.py file in it. You cannot change that, and you don't want to, because it would confuse the heck out of anyone else with some Python experience trying to read your code. When in Rome...

A class is not a module, but it always is inside of a module, and you can import it from there in another module if you need it. So Python modules tend to be much more fine-grained and hierarchical than e.g. C++ namespaces, where you often have many files declaring things in the same namespace. But C++ is a bit of an odd duck in that respect, Java maps packages to directories too for example. (And Java enforces one-class-per-file, which I personally think is a bit much, and I'm happy that Python will let me put a few auxiliary classes together with the main one in a file.)

If you want to have a complex implementation spread out across many files, but want to have a single user-facing namespace API (as you usually do), then you import the public symbols from the various modules in which they're defined into your top-level package's __init__.py, so that they can be imported from there too, and then you tell your users to do just that. We have such a set-up here already (although perhaps transiflow.interface is also supposed to be public?).

Sbte commented 5 months ago

So I either put everything in the __init__.py file, or I leave it as is (possibly with lower case file names, but titlecase class names). As I don't want to expose the "modules" to the users, I don't think it's an issue either way.

LourensVeen commented 5 months ago

You could put everything in the __init__.py directly, but it would end up a very large file, and I'd consider it bad style.

If you want to signal to the user that they should not import something from a particular file/module, then the standard way to do that is to give it a name starting with a single underscore (this works for entire modules, packages, or individual symbols).

That doesn't actually make it private, and Python won't complain if they import it anyway, but if you change the code and release a new version that changes something in such a file then you can rightfully say that it's their own fault their code is now broken, as they knew it wasn't part of the API :smile:.