Improve exported plugin docstrings and annotations

[JSCU-CNI]

This PR improves uniformity across all exported plugin functions. A new test in tests/test_plugin.py (b3539e14958b15490d0c7a48950adc1d21d04cb9) runs the following checks when iterating over find_plugin_functions.

Plugin methods should:

1. specify what they return
2. have a return annotation
3. have a valid rst docstring

Plugin classes are also checked for valid rst docstrings.

Further more we think the tox -e docs-build command should be extended with the -W argument, making doc builds fail when a warning is generated by Sphinx. We have suppressed certain warnings as can be seen in tests/_docs/conf.py, which should make the build warning log more clear. We can add this argument as soon as a PR in sphinx-argparse-cli is merged (https://github.com/tox-dev/sphinx-argparse-cli/pull/171).

Please let us know what you think of this improvement. This will certainly help our internal build and development process as it forces plugin developers to be more concise when defining export functions.

[cecinestpasunepipe]

Thanks for the improvements. Looks really good! We will discuss it with the team and come back to you.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 98.37838% with 3 lines in your changes missing coverage. Please review.

Project coverage is 76.75%. Comparing base (3f93c40) to head (f080f44). Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
dissect/target/helpers/mount.py	0.00%	1 Missing :warning:
dissect/target/plugin.py	50.00%	1 Missing :warning:
dissect/target/plugins/os/windows/ual.py	90.90%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
+ Coverage   76.69%   76.75%   +0.06%     
==========================================
  Files         316      315       -1     
  Lines       27109    27126      +17     
==========================================
+ Hits        20790    20820      +30     
+ Misses       6319     6306      -13     

Flag	Coverage Δ
unittests	76.75% <98.37%> (+0.06%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[JSCU-CNI]

sphinx-argparse-cli version 1.16.0 has been released with our fix for a persistent warning which is now removed. Using the -W argument should now be possible in this branch. Could you let us know what the thoughts of the dissect team on this PR are?

[JSCU-CNI]

Thanks for the quick review! We've applied all suggestions where possible in b1e5a03.

[JSCU-CNI]

Thanks for the additional review! It seems like we are still left with the following tox -e docs-build warnings. The last two we can fix, the first two are unknown to me. They seem to originate from sphinx-autoapi.

WARNING: Unknown type: placeholder
WARNING: Unknown type: placeholder
/tmp/dissect.target/tests/_docs/api/dissect/target/helpers/configutil/index.rst:101: WARNING: duplicate object description of dissect.target.helpers.configutil.ConfigurationParser.parsed_data, other instance in api/dissect/target/helpers/configutil/index, use :no-index: for one of them
/tmp/dissect.target/tests/_docs/api/dissect/target/filesystems/config/index.rst:116: WARNING: duplicate object description of dissect.target.filesystems.config.ConfigurationEntry.parser_items, other instance in api/dissect/target/filesystems/config/index, use :no-index: for one of them

Once these are fixed (or ignored?) we can enable the -W or --fail-on-warning flag in tests/_docs/Makefile.

[Schamper]

Thanks for the additional review! It seems like we are still left with the following tox -e docs-build warnings. The last two we can fix, the first two are unknown to me. They seem to originate from sphinx-autoapi.

WARNING: Unknown type: placeholder
WARNING: Unknown type: placeholder
/tmp/dissect.target/tests/_docs/api/dissect/target/helpers/configutil/index.rst:101: WARNING: duplicate object description of dissect.target.helpers.configutil.ConfigurationParser.parsed_data, other instance in api/dissect/target/helpers/configutil/index, use :no-index: for one of them
/tmp/dissect.target/tests/_docs/api/dissect/target/filesystems/config/index.rst:116: WARNING: duplicate object description of dissect.target.filesystems.config.ConfigurationEntry.parser_items, other instance in api/dissect/target/filesystems/config/index, use :no-index: for one of them

Once these are fixed (or ignored?) we can enable the -W or --fail-on-warning flag in tests/_docs/Makefile.

Did https://github.com/fox-it/dissect.target/pull/725/commits/13cf9fbac77f849d70efc063de69cb73fbe0f863 fix that? So can -W be added?

[JSCU-CNI]

Did https://github.com/fox-it/dissect.target/commit/13cf9fbac77f849d70efc063de69cb73fbe0f863 fix that? So can -W be added?

Unfortunately not. It seems like these warnings can be caused by multiple things: https://github.com/readthedocs/sphinx-autoapi/issues/180

[JSCU-CNI]

Using the following modified sphinx build options we are able to dive into this issue a little deeper:

SPHINXOPTS    ?= -vvv -jauto -w $(BUILDDIR)/warnings.log --fail-on-warning --show-traceback

It seems that these placeholder warnings are caused by the following files:

• dissect/target/plugins/os/unix/etc.py
• dissect/target/plugins/general/config.py
• dissect/target/plugins/os/unix/etc/etc.py

The class EtcTree is registered twice in os.unix.etc and os.unix.etc.etc. Removing dissect/target/plugins/os/unix/etc.py fixes this issue. I am not well versed in the Etc plugin structure, @Schamper do you think it makes sense to delete the os/unix.etc.py file in favor of os/unix/etc/etc.py?

[Schamper]

I don't really understand how it works either :smile: @Miauwkeru can you comment on the above regarding the EtcPlugin?

[Miauwkeru]

I don't really understand how it works either 😄 @Miauwkeru can you comment on the above regarding the EtcPlugin?

the dissect/target/plugins/os/unix/etc.py seems to be a leftover that should have been replaced with etc/etc.py. Cause it makes no sense having both. @JSCU-CNI os/unix/etc.py can be removed

[JSCU-CNI]

Thanks for clarifying @Miauwkeru. We have removed the file in c34a00a.

[JSCU-CNI]

Could this PR be looked at soon? I am not looking forward to keep updating and fixing the issues the new tests find in new functions. The sooner this is in the main test suite the better :)