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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: Improving documentation #525

Draft
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

mich41v4294
Copy link

@mich41v4294 mich41v4294 commented Apr 19, 2024

PR Focused on improving documentation to ease the learning curve for newcomers, since a lot of documentation was lacking.

Edited rendered documentation available at ReadTheDocs.

@rumpelsepp
Copy link
Member

Thank you very much for your effort and your pull request! Very much appreciated!

There are a few remarks:

  • We avoid documenting, e.g., command line flags in the documentation text. The reason is that it gets outdated very quickly when the corresponding code is changed. We have limited resources and we try to focus as much as possible. Thus, we document everything (if possible) in the code directly, e.g. in the help text of the argparser for the relevant --help pages.
  • The documentation page "scan_modes" is a high level description of how the scanning works. It is very important for us that this page does not turn into a user manual, as this site is often used as a reference in reports etc. The focus here is "how does it work?" and not "how is it used?".

I find your explanations how particular scanners work useful. However, the syncronization problem code <-> documentation still applies. Looking forward, I propose the following:

  1. Move docstrings that describe how a scanner works to "scan_modes".
  2. Remove duplications from the docstrings, e.g. documentation of command line arguments or options. Update the help strings of the relevant argparser instead. gallia 2.0 will use pydantic containers where the documentation of options, command line arguments or api settings can be placed at a single location in code. We have a tool that will migrate the code with that documentation to the new code architecture.
  3. Remove gallia.commands from the API docs. It is not considered public API and in any case it will change with gallia 2.0. The proposed docstrings can be moved to a (new) page in the sphix documentation.

Thank you for your understanding. I took some time and thought about how to write this answer. I hope it generates only a decent amount of additional work for you!

@mich41v4294
Copy link
Author

I have removed duplicate information from the docstrings, made scan_modes.md focus more on the actual technical workings of the scans so they could be referenced in reports and specific scans understood by third parties without the knowledge of the tool and removed gallia.commands from the API.

I would also like to ask for confirmation, Gallia ISOTP Discover does not support 29bit CAN IDs now at all? Wanted to note that in the ISOTP chapter of scan_modes.md but couldn't get it working.

@rumpelsepp rumpelsepp added this to the gallia 2.0 milestone Aug 12, 2024
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

Successfully merging this pull request may close these issues.

2 participants