Documentation for ssg library

[marcusburghardt]

Description:

This PR consists of several commits, one commit for each file in ssg folder (without considering sub-folders), where Docstrings are included in functions and classes.
I have opted by the Google format since it has a good compatibility with tools and a good readability.

During this work, I reviewed about 365 functions and 40 classes to make sure the Docstring was aligned to the code.
In most of the cases complete Docstring was created, in some cases it was just formatted to be consistent with Google format and in other cases I had to update the content.

In some really simple functions I opted to not include a Docstring because the function was self-explained.
It was also included Docstrings for internal functions when I considered them important to better understand when and how they were used.

I tried to keep the Docstrings as generic as possible but also inclined to the context of the project since some functions without context would be difficult to understand.

Finally, I skipped all files/scripts used to build the content, which start with build_*.py and the content_diff.py file.

Rationale:

Once the PR is merged, the Docstrings should also improve our documentation in https://complianceascode.readthedocs.io/en/latest/modules/ssg.html, making it easier for the community to consume and contribute to the ssg library.

Review Hints:

Be prepared for a time-consuming task, but I would say that it was great to improve my understanding about the SSG library. The work also gave me many insights about what could be improved, but this is definitely out of scope here.

No code was changed, so no risk is expected from this PR. In some isolated cases I removed comments from functions because they were incorporated in Docstrings, so duplication was not necessary. But in general ensured no code changes.

[github-actions]

Start a new ephemeral environment with changes proposed in this pull request:

Fedora Environment

Oracle Linux 8 Environment

[vojtapolasek]

@marcusburghardt just to clarify, are you sure it is a good idea to consolidate documentation for all class members here? Wouldn't it make problems when someone wants to see help for particular method?

[marcusburghardt]

The intention is not to consolidate documentation of methods within the class Docstrings.

Within the Class Docstring there is just a brief of their methods so when the class is called, for example in VSCode IDE it already shows the available methods. Otherwise we need to rely on autocomplete or check the class definition, for example. So it is helpful in this case. It is true that once the class is updated, we should also update the Docstring otherwise there is risk to have outdated Docstring. I would like to hear more opinions if it is worth or not to briefly mention the methods within class Docstrings. What do you think @jan-cerny , @Mab879 ?

Another topic is about functions Docstrings. They should not be replaced by Class Docstrings, but in some cases, as I mentioned in the PR description, I opted to not create a Docstring because the one line function was self explained.

[Mab879]

Keeping the list of methods up-to-date is one concern I have about listing the method in the class docstring,

[marcusburghardt]

I would like to move forward with this. Considering at least one case where methods mentioned in class Docstrings is useful for developers and the risk of misalignment between Docstrings and methods along the time, I would prefer to keep this for now since they are already there.

We can mitigate the risk of misalignment by exercising due diligence both when developing and when reviewing code. If this becomes a real problem in the future, we can always reevaluate and perhaps remove method information from class Docstrings.

[vojtapolasek]

Hello,
I must say I am not in favor of this. I see the benefit of having everything at the one place, so that when you open documentation for the class, you see methods as well. However, I have following concerns:

1. There are specific facilities exactly for this, without writing it in the code directly. For example, VS Code has the "outline" view, see the screenshot. It is very configurable I would say.
2. It is not standard.
3. That means, that people are not used to that and we will have to keep extra eye on consistency between the real method definition and the definition in the class documentation.

In case you stand strongly behind this desire, I suggest you propose a separate PR adding this documentation to classes together with update of our style guide.

[vojtapolasek]

Adding the screenshot here.

[marcusburghardt]

This example of "outline" is similar to what I mentioned before, regarding autocomplete. The same information in your "outline" example can be obtained in different ways, but not as straightforward as in Docstrings.

In my assessment, having this offers more pros than cons. I don't believe that in this case we should remove this helpful information only because there is an hypothetical risk of having something outdated along the time, which is something we can pretty much mitigate. Also, this SSG package is relatively stable if we look the changes along the last years. In addition, I saw multiple cases of Docstrings not mentioning all methods in different projects and I found the methods anyways with some extra steps. It can happen, but what is the real problem of not having a class Docstring perfectly updated about a new method included or considerably changed, specially if the methods have their own Docstrings, which are more complete?

I don't see an strong argument against having this information there for who wants to use them among other options to obtain the same information. The intention of the documentation is to make ssg easier to be consumed and I am in favor of giving options for different preferences, specially in this case where no code is impacted.

But I want to move forward with the documentation. So, let me know if this is blocking the PR to be merged. If so, I will work on an additional commit to remove the information. If you believe this should be removed, I am fine to remove and unblock it. In any case, thanks for the feedback on this.

[vojtapolasek]

I admit that having this section in the class documentation is handy, because it is just next to other information about the class, but I am still not convinced that pros outweight cons. As I am not the only one having concerns around this change, please propose it as a separate PR, preferably with an ammendment to the style guide.

[marcusburghardt]

Removed in my last commit. I am personally in favor of including documentation that is helpful for some people even if not everybody is expected to use the same information, so we can support a wider audience. Specially when the information causes no harm to the project. But I removed the information so we can move forward with this PR and discuss preferences in another place. : )

[vojtapolasek]

Hello @marcusburghardt and thank you for this large update. It is a big piece of work. I read through it and I have mostly minor comments, I put them near to relevant lines.
One more thing to be resolved is documentation of methods within the class documentation, let's discuss it in the particular thread which already exists.

[vojtapolasek]

I am not sure if it is a good idea to have these methods listed here. I understand it might be convenient to have nice list of methods just under the class description, but I am afraid this list and real list of methods (not talking about their parameters and meaning) can get out of sync.

[vojtapolasek]

One more thing which could but does not need to be part of this PR is updating of style guide adding a bullet point that new functions should be documented.

[marcusburghardt]

One more thing which could but does not need to be part of this PR is updating of style guide adding a bullet point that new functions should be documented.

I plan to create a separate PR for this.

[codeclimate]

Code Climate has analyzed commit 2c71e1e and detected 92 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	4
Duplication	10
Style	78

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 60.9% (0.0% change).

View more on Code Climate.

[marcusburghardt]

/packit test

[vojtapolasek]

/packit retest-failed

[vojtapolasek]

Thank you for all the changes. I am merging the PR.
Failing tests are not relevant for these changes, they are only doc string changes anyway.