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

DOXYGEN: Add doxygen groups to header files in the common folder #2361

Merged
merged 1 commit into from Oct 4, 2020

Conversation

@b-gent
Copy link
Contributor

@b-gent b-gent commented Jul 8, 2020

All the header files from the common folder now have doxygen groups added to them. Thanks to that, the usability of API doc is greatly improved and crucial information can be found. Compare:
Old:
image
New:
image

@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Jul 8, 2020

@criezy
Copy link
Member

@criezy criezy commented Jul 8, 2020

  1. I couldn't find the doxygen config file used for your current API doc anywhere in the repository

You can find the current doxygen config file in https://github.com/scummvm/scummvm-sites/tree/doxygen

And indeed there is a lot of work needed to organise our doxygen documentation and make it usable. This looks like a good start. Using groups is a good idea and the main page could for example be improved with some basic information and link to https://wiki.scummvm.org/index.php?title=Developer_Central.

@Mataniko
Copy link
Contributor

@Mataniko Mataniko commented Jul 9, 2020

Thanks for the PR, Looking forward to seeing your application!

@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Jul 9, 2020

  1. I couldn't find the doxygen config file used for your current API doc anywhere in the repository

You can find the current doxygen config file in https://github.com/scummvm/scummvm-sites/tree/doxygen

I wasn't aware of the scummvm-sites fork, thanks :)

And indeed there is a lot of work needed to organise our doxygen documentation and make it usable. This looks like a good start. Using groups is a good idea and the main page could for example be improved with some basic information and link to https://wiki.scummvm.org/index.php?title=Developer_Central.

Yes, and not only the main page, I bet there is much more 'conceptual' documentation that could be added to the doxygen docset that would explain how the API works. The empty placeholder pages that are there right now that are supposed to explain how to implenent a plugin are an example of this.

Adding @defgroup and @InGroup doxygen tags into all headers
in the common folder that contain doxygen blocks.

This improves the structure, readability, and findability
of information in the resulting output.

This commit targets purely structure and does not deal with
the content of the currently existing doxygen documentation.
@b-gent b-gent force-pushed the b-gent:season_of_docs branch from 177bdd1 to 13d464e Sep 9, 2020
@b-gent b-gent marked this pull request as ready for review Sep 9, 2020
@b-gent b-gent changed the title DOC: Improve API doc DOXYGEN: Add doxygen groups to header files in the common folder Sep 9, 2020
@Mataniko Mataniko requested review from Mataniko and sev- Sep 12, 2020
* @defgroup common_array Arrays
* @ingroup common
*
* @brief Functions for working on arrays.

This comment has been minimized.

@Mataniko

Mataniko Sep 13, 2020
Contributor

these are functions to replace some std arrays

This comment has been minimized.

@b-gent

b-gent Sep 15, 2020
Author Contributor

I'll correct this (as soon as we agree on terminology).

This comment has been minimized.

@criezy

criezy Sep 30, 2020
Member

Actually these are classes and not functions.

* @defgroup common_md5 MD5 checksum
* @ingroup common
*
* @brief API for computing the MD5 checksum.

This comment has been minimized.

@Mataniko

Mataniko Sep 13, 2020
Contributor

This is less of an API and more of a method/class - @sev- WDYT on terminology in general

This comment has been minimized.

@b-gent

b-gent Sep 15, 2020
Author Contributor

That's a good point about having consistent terminology. As I see it, 'API' is a superset of whatever we might have in these header files - classes, methods, functions, structures etc. If you agree with this, then probably using the term 'API' is safest and most future-proof.

This comment has been minimized.

@criezy

criezy Sep 30, 2020
Member

In this specific case I would probably have used Functions if I had written the documentation. But API is also fine with me. And indeed we need consistency.

* @defgroup common_memory Memory
* @ingroup common
*
* @brief Functions for managing the memory.

This comment has been minimized.

@Mataniko

Mataniko Sep 13, 2020
Contributor

Same as earlier - we should have consistent terminology - functions, api, class, methods

This comment has been minimized.

@b-gent

b-gent Sep 15, 2020
Author Contributor

I guess in this case the most accurate description is 'Class templates for managing the memory' ?

This comment has been minimized.

@criezy

criezy Sep 30, 2020
Member

Referring to classes here seems confusing to me. They look like Template functions for managing the memory of a container.

@Mataniko
Copy link
Contributor

@Mataniko Mataniko commented Sep 13, 2020

Overall looks like a massive improvement - left a couple comments on terminology that we should standardize before proceeding

@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Sep 15, 2020

I would like to get this in as soon as possible. I'm already making additional changes to these files on another branch and I would like to avoid some crazy merge conflicts.

@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Sep 18, 2020

The output is now available for preview on https://b-gent.github.io/PR2361/group__common.html

@sev- , it would be great if you could have a look at the comments here and the PR as a whole.

@sev-
Copy link
Member

@sev- sev- commented Oct 4, 2020

Great work, merging

@sev- sev- merged commit ec24687 into scummvm:master Oct 4, 2020
5 checks passed
5 checks passed
Windows (win32, x86-windows, x86, --enable-faad --enable-mpeg2 --disable-fribidi, curl faad2 flui...
Details
Windows (x64, x64, x64-windows, --enable-faad --enable-mpeg2 --disable-fribidi, curl faad2 fluids...
Details
Windows (arm64, arm64, arm64-windows, --enable-faad --enable-mpeg2 --disable-fribidi, curl faad2 ...
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.