chore: Add docstrings and types to Version class

[holmanb]

Additional Context

chore: Add docstrings and types to Version class

[blackboxsw]

I like that you are following google's python style guide: https://google.github.io/styleguide/pyguide.html

Let's avoid extra empty newlines in the docstrings as we are dedicating a lot of vertical real-estate to these docstrings already.

[TheRealFalcon]

I have thoughts...let's not merge this quite yet

[TheRealFalcon]

nit: If we change >> to >>>, these would be valid doctest examples. We don't use it, but it'd be an easy way to gain test coverage for utility functions in the future and ensure our docstring examples are kept up to date.

More broadly, if we're wanting to standardize on a format, I'd prefer we use the Sphinx format. Reasons:

1. Since we're already using Sphinx, if we ever decide to autogenerate docs docstrings, it should be a fairly easy transition.
2. Since the format is very similar to the epytext strings that are scattered through the codebase, it'd be a much simpler find/replace for those than switching them to Google format would be.
3. Minor point, but Google convention technically breaks pep-8/pep-257 in things like recommending non-imperative mood. If we use a linter and say "use google style", our currently existing docstrings that use imperative mood may not match.

Since existing tooling generally supports both formats, I don't think there's any loss from not using Google format.

[holmanb]

nit: If we change >> to >>>, these would be valid doctest examples. We don't use it, but it'd be an easy way to gain test coverage for utility functions in the future and ensure our docstring examples are kept up to date.

No objections from me for cheap test coverage :D

More broadly, if we're wanting to standardize on a format, I'd prefer we use the Sphinx format. Reasons:

1. Since we're already using Sphinx, if we ever decide to autogenerate docs docstrings, it should be a fairly easy transition.

2. Since the format is very similar to the epytext strings that are scattered through the codebase, it'd be a much simpler find/replace for those than switching them to Google format would be.

3. Minor point, but Google convention technically breaks pep-8/pep-257 in things like recommending non-imperative mood. If we use a linter and say "use google style", our currently existing docstrings that use imperative mood may not match.

Since existing tooling generally supports both formats, I don't think there's any loss from not using Google format.

Good points.

The only reservations that I have are that sphinx has a little more boilerplate and a little more work for my eyes to parse than google-styled docstrings. I'm sure I'll get used to sphinx docstrings without issue. I'll update this PR unless @blackboxsw or @aciba90 has any strong objections.

Lets open a PR or file an issue to make sure that we eventually get this documented and update the current docstrings (and maybe a CI job for doctest while we're at it?)

[TheRealFalcon]

boilerplate

I don't think we need to include every field every time. E.g., there's a stackoverflow answer that has this:

"""
This is a reST style.

:param param1: this is a first param
:param param2: this is a second param
:returns: this is a description of what is returned
:raises keyError: raises an exception
"""

Which is a lot less boilerplate. Since we (ideally) have types specific in the function definition, we shouldn't also need them in the docstring.

[aciba90]

Thanks for the discussion.

I do not care much about which docstring format we use, but more about picking up one and be consistent with it.

If we choose a new one, we could use https://github.com/dadadel/pyment to convert existent docstrings if we want.

To lint / enforce docstrings to follow a particular style, we could use https://docs.astral.sh/ruff/settings/#lint_pydocstyle_convention, since https://github.com/PyCQA/pydocstyle has been deprecated in favor of ruff.

[holmanb]

If we choose a new one, we could use https://github.com/dadadel/pyment to convert existent docstrings if we want.

I can see this being useful to get started with some modules that are more important. These are the ones that I would start with

stages.py/main.py -> because it defines core logic
util.py -> because is so heavily used across the codebase
distros/__init__.py -> because it defines an interface which gets implemented by all distros
sources/__init__.py -> because it defines an interface which gets implemented by all platforms

To lint / enforce docstrings to follow a particular style, we could use https://docs.astral.sh/ruff/settings/#lint_pydocstyle_convention, since https://github.com/PyCQA/pydocstyle has been deprecated in favor of ruff.

+1

[holmanb]

To lint / enforce docstrings to follow a particular style, we could use https://docs.astral.sh/ruff/settings/#lint_pydocstyle_convention, since https://github.com/PyCQA/pydocstyle has been deprecated in favor of ruff.

+1

@aciba90 fyi #5277

[aciba90]

To lint / enforce docstrings to follow a particular style, we could use https://docs.astral.sh/ruff/settings/#lint_pydocstyle_convention, since https://github.com/PyCQA/pydocstyle has been deprecated in favor of ruff.

+1

@aciba90 fyi #5277

Thanks for pinging!

@holmanb I went into a rabbit hole: canonical/pycloudlib#374

[holmanb]

@TheRealFalcon @aciba90 @blackboxsw I just updated with a sphinx docstring fyi

[TheRealFalcon]

This version LGTM, but lets give @blackboxsw and @aciba90 time for any additional bikeshedding.

[aciba90]

LGTM, thanks!

[github-actions]

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

[holmanb]

@aciba90 I misread your earlier comment, my apologies. I thought that this was suggesting to add docstrings by default to all functions, but now that I've re-read I understand what you said.

If we choose a new one, we could use https://github.com/dadadel/pyment to convert existent docstrings if we want.

Yes 100% agreed. I think that we should do a bulk update of all comments in the project (or at least in cloudinit/) to match this format. Piecemeal updates will take forever. I just filed #5346 to track this effort.