Ultimaker / Cura

3D printer / slicing GUI built on top of the Uranium framework
GNU Lesser General Public License v3.0
6.12k stars 2.07k forks source link

Documentation should be written as docstrings #5512

Closed kfsone closed 4 years ago

kfsone commented 5 years ago

Python is a dynamic language, strong python developers spend a lot of time in the repl or iPython or notebooks.

The cura code is well commented, but the correct form of documentation in Python is to put the class/function description in a "docstring".

## The Arrange classed is used together with ShapeArray. Use it to find
#   good locations for objects that you try to put on a build place.
# 
#   Note: Make sure the scale is the same between ShapeArray objects and the Arrange instance.
class Arrange:

    #...

    ## Helper to create an Arranger instance
    # 
    #   Either fill in scene_root and create will find all sliceable nodes by itself,
    #   or use fixed_nodes to provide the nodes yourself.
    #   \param scene_root   Root for finding all scene nodes
    #   \param fixed_nodes  Scene nodes to be placed
    @classmethod
    def create(cls, scene_root = None, fixed_nodes = None, scale = 0.5, x = 350, y = 250, min_offset = 8):
        pass

>>> help(Arrange)
class Arrange(builtins.object)
|  Class methods defined here:
|
|  create(scene_root=None, fixed_nodes=None, scale=0.5, x=350, y=250, min_offset=8) from builtins.type
|
 |  ----------------------------------------------------------------------
 |  Data descriptors defined here:
 |
 |  __dict__
 |      dictionary for instance variables (if defined)
 |
 |  __weakref__
 |      list of weak references to the object (if defined)

Using docstrings would greatly aide external developers looking to work with/on the code:

class Arrange:
    """Used together with ShapeArray to organize objects on the build plate.

    The Arrange class is used together with ShapeArray. Use it to find good
    locations for objects that you try to put on a build plate.

    \note Make sure the scale is the same between ShapeArray objects and the Arrange instance.
    """

    # ...

    @classmethod
    def create(cls, scene_root = None, fixed_nodes = None, scale = 0.5, x = 350, y = 250, min_offset = 8):
        """ Helper to create an Arrange instance.

        Either fill in scene_root and create will find all sliceable nodes by itself,
        or use fixed_nodes to provide the nodes yourself.

        \param scene_root   Root for finding all scene nodes
        \param fixed_nodes  Scene nodes to be placed
        """
        pass

>>> help(Arrange)
Help on class Arrange in module __main__:

class Arrange(builtins.object)
 |  Used together with ShapeArray to organize objects on the build plate.
 |
 |      The Arrange class is used together with ShapeArray. Use it to find good
 |      locations for objects that you try to put on a build plate.
 |
 |
 |  ote Make sure the scale is the same between ShapeArray objects and the Arrange instance.
 |
 |  Class methods defined here:
 |
 |  create(scene_root=None, fixed_nodes=None, scale=0.5, x=350, y=250, min_offset=8) from builtins.type
 |      Helper to create an Arrange instance.
 |
 |      Either fill in scene_root and create will find all sliceable nodes by itself,
 |      or use fixed_nodes to provide the nodes yourself.
 |
 |      \param scene_root   Root for finding all scene nodes
 |      \param fixed_nodes  Scene nodes to be placed
 |
 |  ----------------------------------------------------------------------
 |  Data descriptors defined here:
 |
 |  __dict__
 |      dictionary for instance variables (if defined)
 |
 |  __weakref__
 |      list of weak references to the object (if defined)
>>> help(Arrange.create)
Help on method create in module __main__:

create(scene_root=None, fixed_nodes=None, scale=0.5, x=350, y=250, min_offset=8) method of builtins.type instance
    Helper to create an Arrange instance.

    Either fill in scene_root and create will find all sliceable nodes by itself,
    or use fixed_nodes to provide the nodes yourself.

    \param scene_root   Root for finding all scene nodes
    \param fixed_nodes  Scene nodes to be placed

As an engineer with 20+ years of C++ experience, I can relate to how wrong it feels to put the comments on the inside, but consider this:

While there are some IDEs that will recognize the c-style prefix comment for tooltips etc, they are mostly all C/C++ oriented and not particularly helpful with Python.

Clearly the documents are written to be surfaced in some form of tooling - but how often do you build and use the doxygen documentation, or do you just go to the function and read it?

The latter is my default, as a C++ engineer, but all the tooling that gives the best Python productivity - from VS Code to "Kite" - depends on docstrings, not comments.

Which is better: Python code that looks more like C code to the confusion of all, or Python code that surfaces documentation superbly to Python engineers while being aesthetically displeasing to myself and every other C-based engineer on the planet?

rburema commented 5 years ago

Our code-conventions/style state that we should be able to run Doxygen on the code though. However, considering that there's a 'Todo' attached to that sub-heading, maybe we should look at it again.

(Note that those conventions linked are the general ones. The Python specific one, which uses this as a base, doesn't say much about comments. Which makes this the leading document.)

kfsone commented 5 years ago

I should note that you don't need/use Doxygen to surface this in Python: it's simply a built-in feature of the language that if an object or class has a string as the first line of it's definition, that will be assigned to the entity's "docstring" field during parsing.

Open an ipython (or if you don't have it) python prompt and try the following:

>>> def a(i):
...   """Invert the world and convert i to a std::vector or std::maybe's"""
...   pass
...
>>> help(a)
# ... output
>>> import dis ; help(dis.dis)
# ... output

It's pervasive:

$ pip install --user zeroconf ; python -c "import zeroconf ; help(zeroconf)"
$ pip install --user pillow ; python -c "import PIL; help(PIL)"

You're already paying the price of writing well-formed comments to an end you don't leverage to no benefit. The docstring approach is part of Python's built-in exhaustive reflection. Note that the doc-string gets its own IDE annotation in this VS Code example:

image

So if I am good-old reading the code, despite being what feels like the wrong side of the definition, it quickly becomes a lot easier to spot them, identify them, read them, etc.

If it's an acceptable style change, then I'll go ahead and convert them in a fork - but it's one of those things you don't do if it's not going to float with the developers/maintainers :)

Allow me to explain the relevance:

Since caving in and learning Python 8 years ago, I've found that the answers to the question "wtf is anyone using python" are primarily "ipython" and "jupyter". "ipython" is basically an extended version of the regular python repl with proper readline support, history editing, along with advanced "magic" features like '%time' and '%timeit' for benchmarking code - which go superbly hand-in-hand with Python's built-in "dis" library (for seeing the op-code functions generate). ipython also caches the results of every statement; so you can run commands in "show me what you do" mode without having to write prints all the time:

# perl
$a = get_the_thing();
print("$a\n");

vs

> ipython
Python 3.7.1 (default, Dec 10 2018, 22:54:23) [MSC v.1915 64 bit (AMD64)]
Type 'copyright', 'credits' or 'license' for more information
IPython 7.2.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: def get_the_thing(): return 'world'

In [2]: %timeit get_the_thing()
48.6 ns ± 1.63 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [3]: get_the_thing()
Out[3]: 'world'

In [4]: 'hello ' + _3
Out[4]: 'hello world'

"jupyter" is a multi-language web-oriented version of it that has exploded in the big-data, mechanical and scientific computing sectors (people who like R for math and python for the other stuff)

Large sectors of industries that do a lot of 3d printing.

rburema commented 5 years ago

Seems like PyCharm supports it as well. I'll bring it up tomorrow. Even if we (our team) decide to go with it though (and there might be reasons why we won't); keep in mind that the Cura team(s) is(are) not the only one(s) using these standards, or even Python, at Ultimaker, so these decisions can take time and might be somewhat conservative. (Because a larger group of people needs to accept a potential change to an existing standard.)

kfsone commented 5 years ago

Understood - that's why I've presented the extra data. Four-year-ago me would beat today-me senseless with a stick for suggesting the same thing, until I started on a meatier project to beef up my Python for a job a position at Facebook: http://bitbucket.org/kfsone/tradedangerous

DocString specification: https://www.python.org/dev/peps/pep-0257/

kfsone commented 5 years ago

I also just finished spending 2 years convincing some really great engineers that C++14 had a few advantages over C++98 ;)

Again - I'd be happy to do the migration in a fork. It may also be worth your looking at some of the major Python development productivity tools:

$ or > pip install --user jupyter
$ or >jupyter notebook
< opens web browser >
In Cell 1 type:
# Call this function if you want to be a 
# 
# \param user The person to convert to a millionaire
# \param score Set FALSE to award money
def winner(user, score=True):
  if score is False:
    user.bank = millions
  else:
    user.bank = score
(hit shift-enter to execute the code and move to/create the next cell)
(in cell 2 type:)
help(winner)
(hit shift-enter to execute)
now re-select the first cell and switch it to a docstring:
def winner(user, score=True):
  """ Call this function if you want to be a 

   \param user The person to convert to a millionaire
   \param score Set FALSE to award money
  """
  if score is False:
    user.bank = millions
  else:
    user.bank = score
(hit shift-enter to re-run this cell, then hit shift-enter to rerun the second (help) cell)

and you may want to take a look at https://kite.com/ (cross-ide intellisense capable of completing entire blocks of code, works in code/pycharm/vstudio even vim; c++ support in the works)

Ghostkeeper commented 5 years ago

We know about docstring and that it's the standard for Python. That's not the problem. The problem is that Doxygen doesn't support docstring as well as the format we're using now. It's unable to process any of the Doxygen commands (source).

That's why we're not using Docstring, but the one format in which Doxygen does support commands.

We're locked into Doxygen rather than another documentation tool because our software department works with multiple languages rather than just Python.

kfsone commented 5 years ago

https://pypi.org/project/doxypypy/This is a filter for doxygen, I'd forgotten I'd even had to bother with it back when.Making python source look less like c (or java or c#) is a great way to avoid falling into some of python's worst pitfalls...pythonfor i in iterable:  self.mesh.vertex.coordinates.adjust(i[0], i[1])because you thought "the compiler will hoist those lookups out of the loop" ;)python# akin to auto& c = mesh.vtx.coords; for...coord = self.mesh.vertex.coordinatesfor i in iterable:  coord.adjust(i[0], i[1])# more pythonic - thinking to ref a call requires being in a pythonic headspace (and somehow still breathing)adjust = self.mesh.vertex.coordinates.adjustfor x, y in iterable:  adjust(x, y)# correct:self.mesh.vertex.coordinates.adjust(iterable)

ninovanhooff commented 4 years ago

Raising this from the dead because I passionately agree and have been able to convince the team to switch to rst docstrings. We would welcome help from the community in this effort. @kfsone , are you still around and Willing to answer the call?

Ghostkeeper commented 4 years ago

The transition is completed now.