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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve docstrings in classes & functions for API #225

Open
tomschr opened this issue Feb 8, 2020 · 3 comments
Open

Improve docstrings in classes & functions for API #225

tomschr opened this issue Feb 8, 2020 · 3 comments
Milestone

Comments

@tomschr
Copy link
Contributor

tomschr commented Feb 8, 2020

Situation

Currently, the situation with docstrings in classes and functions seems to me ambiguous and inconsistent. 馃槈 Some are pretty well documented, others are not.
Additionally, some signatures contain conflicting information between docstrings and type annotations, for example:

# file ics/icalendar.py
class Calendar(Component):
    # [...]
    def __init__(
        self,
        imports: Union[str, Container] = None,
        events: Iterable[Event] = None,
        todos: Iterable[Todo] = None,
        creator: str = None
    ):
        """Instantiates a new Calendar.

        Args:
            imports (**str**): data to be imported into the Calendar,
            events (**Set[Event]**): Events to be added to the calendar
            todos (Set[Todo]): Todos to be added to the calendar
            creator (string): uid of the creator program.

        If ``imports`` is specified, every other argument will be ignored.
        """

Compare the arguments imports, events, and todos in the docstring and with the type annotation. Both are advertised differently. This is one of the worst situations when documenting stuff: conflicting information. IMHO this should be improved to avoid confusing our developers.

Questions

  1. Why are types both included into docstring and type annotations? This seems to be double work. Shouldn't there be only one location?
  2. Shouldn't the types automatically inserted when building the documentation using the sphinx-autodoc-typehints plugin?
  3. Shouldn't have every file a global docstring about its purpose, license, probably dependencies, author(s) etc.?
  4. Why are some classes included in the API documentation and others are not? For example, the class Organizer in file organizer.py, Timeline in timeline.py and others.

Proposed Solution

  1. Create a consistent docstring "spec" to improve consistency for every function. For example, a docstring should contain (at least):

    • the purpose of the function/class member/etc.
    • a description of its arguments
    • the return value, if applicable
    • does the function raises exceptions?
    • any dependencies? Restrictions?
    • In some cases, examples could help.
  2. We should check if we could rephrase docstring which contains the pipe symbol (|); it should be avoided due to consistency reasons (interactive Python shell).

  3. Document the missing pieces to improve API documentation.

  4. Avoid ambiguous types between docstring and type annotation. Ideally, type annotations are included into the API documentation without explicitly mention it in the docstring.

@C4ptainCrunch
Copy link
Member

Answers to your questions

  1. This is solely for historical reasons when we did not have type hints
  2. Yes, it would be great :)
  3. 馃憤
  4. I guess it's just laziness from myself

Your proposed solutions seems good to me 馃憣

@N-Coder
Copy link
Member

N-Coder commented Feb 12, 2020

@tomschr are there any practical, go-to docstring guidelines (maybe even with an accompanying tutorial specifically for documenting python functions, as I haven't used sphinx before) we could reuse? I'm somewhat intimidated by all the features, markups and outputs. 馃

@tomschr
Copy link
Contributor Author

tomschr commented Feb 12, 2020

@N-Coder Well, normally if you need any information regarding docstrings, the first go-to place would be the PEPs PEP 257 (Docstring Conventions) and PEP 287 (reStructuredText Docstring Format).

However, both are a bit too technical and academic for a go-to docstring guideline. A more helpful approach is RealPython's article Documenting Python Code.

In this article, they give recommendations for classes, module, and package docstrings and show some formats to use inside it. I think it's very helpful. They even referenced Divio's blog, a link that you recently shared. 馃槃

Regarding Sphinx, yes, it can be intimidating. However, you really need very few markup only. If you use the RST format (also used in Python and this library), I normally use:

  • :param NAME: describes the argument
  • :return: describes the return value (not necessarily its datatype).
  • :raises: lists possible exception classes

That's it! Of course, there are more, but in most cases it's enough for a function. You don't mention datatypes, as they are automatically added when using type annotations. Very convenient!

This is an example from the python-semver library, slightly redacted:

def parse(version: str) -> dict:
    """
    Parse version to major, minor, patch, pre-release, build parts.

    :param version: a version string
    :return: dictionary with the keys 'build', 'major', 'minor', 'patch', and 'prerelease'.
         The prerelease or build keys can be None if not provided
   :raises: ValueError, if the version string contains an invalid semver version

   Example:
   >>> ver = semver.parse('3.4.5-pre.2+build.4')
   >>> ver['major']
   3
   >>> ver['minor']
   4
   """
   # ...

I think, the most important tip that I can give here is: be consistent. It doesn't matter much to use fancy markup, but with the macros above, you can document a lot. If needed, you can always add more.

@N-Coder N-Coder mentioned this issue May 16, 2020
21 tasks
@N-Coder N-Coder added this to the Version 0.8 milestone May 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants