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 Documentation #20

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

marcosfpr
Copy link
Contributor

I'm starting to develop the Doxygen documentation for Small. Currently, I've just managed to get the structure done.

For the following steps, I'd fix some issues that may appear on this structure and add new documentation Markdown files to create more documentation as we have in the Futures project.

@marcosfpr marcosfpr force-pushed the update-documentation branch 2 times, most recently from c45e505 to 44bcf1e Compare May 7, 2022 23:54
@alandefreitas
Copy link
Owner

That's really great! We do need to get Doxygen generation properly here.

Did mkdocs work locally for you? Any questions about this structure?

I'll execute all of that locally this week and send you a review. :)

@marcosfpr
Copy link
Contributor Author

@alandefreitas I ran locally, and it works fine :-). Of course, there are a lot of things to get done yet, but it's working at least locally.

@alandefreitas
Copy link
Owner

@alandefreitas I ran locally, and it works fine :-).

Nice. I'm not sure this is mentioned anywhere, but the best way to test and work on it seems to be

mkdocs serve --dirtyreload

The build command is just for CI, where we want the static pages, so we use serve to be able to work on the pages in real time.

--dirty-reload is useful because the references include many pages we don't need to render again when are working on the exposition.

Of course, there are a lot of things to get done yet, but it's working at least locally.

No problem. We just need the basic layout for now.

Copy link
Owner

@alandefreitas alandefreitas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I guess we're almost there. We can work on the content after we get this merged.

docs/.doxybook/templates/breadcrumbs.tmpl Show resolved Hide resolved
docs/index.md Show resolved Hide resolved
.github/workflows/docs.yml Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated

[READ THE DOCUMENTATION FOR A QUICK START AND EXAMPLES](https://alandefreitas.github.io/small/)

</h2>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pages should end with the macros for the doxygen classes (see futures). This allows us to link the library classes in the exposition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I'm not quite sure we have this in the futures' index.md.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. That's a comment assuming the pages would be split.

index.md is the only page for which we don't have

https://github.com/alandefreitas/futures/blob/326957b2bc4238716faecd7a5bff4616d967b4ce/docs/motivation.md?plain=1#L634

because it's just the index. It'll probably never have these references.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I understand. I'll check this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to confess that I didn't understand how these references work. Are those references processed by doxybook2? And the references.md is some kind of special file?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of how it works:

  • First we have the references generated by doxybook2.
  • Doxygen creates references in XML from the javadoc in the header files.
  • Then doxybook2 (plus the templates in .doxybook/templates/) creates a markdown version of the XML in a format similar to cppreference.
  • We append these doxygen/doxybook2 documentation files at the end of the exposition (ref)
  • The beginning of the exposition is still manually written, as usual. But the manual exposition can now (and should now) link to elements in the automatically generated doxygen/doxybook2 documentation.
  • To facilitate linking to elements in the automatic doxygen docs, we create a references.md file with macros for the links we might need recurrently in the manual exposition. These references are appended to all files that might need it.

So, for instance, we can have

[vector]: /small/reference/Classes/classsmall_1_1vector/

in the references.md and (assuming this link is correct), we can just use [vector] in the exposition every time we would just type vector. So whenever the user is reading about small::vector in the exposition, we can click it and go straight to the small::vector reference in /small/reference/Classes/classsmall_1_1vector/.

Of course, these macros don't have to be limited to our reference, and might include links to the C++ reference, other libraries or whatever references we need recurrently in our exposition. So whenever we find something that might be recurring in the exposition, like class and function names, we include it in references and just use the macro in the exposition instead of having to write something like [vector](/small/reference/Classes/classsmall_1_1vector/) over and over again.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is resolved, right?

.github/workflows/docs.yml Outdated Show resolved Hide resolved
docs/CMakeLists.txt Show resolved Hide resolved
# - Quickstart: quickstart.md todo
# - Motivation: motivation.md todo
- Reference: # Reference is organized by module
- Small: reference/Modules/index.md # futures module
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to figure out the best reference structure for this library.

We usually have one module per link in the sidebar, but this library is too small for that. We don't even have oxygen modules defined.

We could create one module/link per container or just a single reference link with all the containers and functions. One of the reference indexes should do for now.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And btw, we need mkdocs serve --dirtyreload to play with the docs and find the best structure.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is resolved, right? I imagine you already got this.

docs/macros.py Show resolved Hide resolved
docs/javascripts/extra.js Show resolved Hide resolved
Copy link
Contributor Author

@marcosfpr marcosfpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some changes. Currently, I'm facing an error with the References generated documentation. The error message is:

 Exception at Utils.cpp:218 Failed to create directory /my/path/to/small/docs/reference/Classes error -1

All the rest is working fine.

.github/workflows/docs.yml Outdated Show resolved Hide resolved
.github/workflows/docs.yml Show resolved Hide resolved
.github/workflows/docs.yml Show resolved Hide resolved
docs/.doxybook/templates/breadcrumbs.tmpl Show resolved Hide resolved
docs/CMakeLists.txt Show resolved Hide resolved
docs/index.md Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated

[READ THE DOCUMENTATION FOR A QUICK START AND EXAMPLES](https://alandefreitas.github.io/small/)

</h2>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I'm not quite sure we have this in the futures' index.md.

mkdocs.yml Show resolved Hide resolved
mkdocs.yml Show resolved Hide resolved
@alandefreitas
Copy link
Owner

I made some changes. Currently, I'm facing an error with the References generated documentation. The error message is:

 Exception at Utils.cpp:218 Failed to create directory /my/path/to/small/docs/reference/Classes error -1

All the rest is working fine.

I'm assuming this happens when running doxybook2, right? You probably just need to remove docs/reference before running doxybook2 again.

Copy link
Owner

@alandefreitas alandefreitas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is almost good to go.

We don't need "Doxygen Documentation - Proposed Changes". Just squashing "Doxygen Documentation" or "[FOLD] Doxygen Documentation" should be OK.

The BG color is nice, but we might want to revisit the colors in stylesheets for the code blocks though. They're not easy to read:

image

The main problem here is I couldn't generate the docs locally either. The failure to create the directory is just a matter of creating the directory locally: matusnovak/doxybook2#40

But then we get

[error] Exception at Doxygen.cpp:210 No <compound> element in file ./xml/index.xml

It seems like the symbols are not going into docs/xml. Something is missing from the Doxyfile. It's output is empty.

One way to debug that independently from the mkdocs toolchain is to temporarily generate the HTML docs:

GENERATE_HTML = YES

in line 31. You should be able to see the reference is empty:

image

README.md Show resolved Hide resolved
mkdocs.yml Show resolved Hide resolved
@marcosfpr
Copy link
Contributor Author

marcosfpr commented May 18, 2022

It looks like this is almost good to go.

We don't need "Doxygen Documentation - Proposed Changes". Just squashing "Doxygen Documentation" or "[FOLD] Doxygen Documentation" should be OK.

The BG color is nice, but we might want to revisit the colors in stylesheets for the code blocks though. They're not easy to read:

image

The main problem here is I couldn't generate the docs locally either. The failure to create the directory is just a matter of creating the directory locally: matusnovak/doxybook2#40

But then we get

[error] Exception at Doxygen.cpp:210 No <compound> element in file ./xml/index.xml

It seems like the symbols are not going into docs/xml. Something is missing from the Doxyfile. It's output is empty.

One way to debug that independently from the mkdocs toolchain is to temporarily generate the HTML docs:

GENERATE_HTML = YES

in line 31. You should be able to see the reference is empty:

image

I figured out what had happened. Basically, you did a folder renaming recently, changing the root project of source/* to include/*, and so it was just a question of renaming some stuff in our scripts as well.

Also, about the color scheme, I made this scheme based on the small logo, but I agree that there are things that need to be changed. Working on that as well.

@marcosfpr marcosfpr force-pushed the update-documentation branch from e74673a to 6d1b74b Compare May 18, 2022 01:55
@alandefreitas
Copy link
Owner

I figured out what had happened. Basically, you did a folder renaming recently, changing the root project of source/* to include/*, and so it was just a question of renaming some stuff in our scripts as well.

Phew

Also, about the color scheme, I made this scheme based on the small logo, but I agree that there are things that need to be changed. Working on that as well.

Yep. We do need a new proper logo on GIMP or something. This was just an SVG template that's been there as a placeholder for longer than it should. I think I opened an issue for that already.

Although the color is circumstantial, this light blue is not that bad. Only the content tabs are kind of hard to read.

Copy link
Owner

@alandefreitas alandefreitas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. We just have the TOC stuff left now.

I think I wasn't clear about my previous [FOLD] comment. We can squash the commit OR add [FOLD] to the following commit.

"[FOLD]" means the second commit should be squashed with the first one. There's no point in "[FOLD]" in a single commit.

Using "[FOLD]" is optional. The reason for using "[FOLD]" is just to make it easier to review incremental changes, if that's the case. Either option is fine.


If contributing with code, please leave all warnings ON (`-DSMALL_BUILD_WITH_PEDANTIC_WARNINGS=ON`), use [cppcheck](http://cppcheck.sourceforge.net/), and [clang-format](https://clang.llvm.org/docs/ClangFormat.html).

If contributing to the documentation, please edit [`README.md`](README.md) directly, as the files in [`./docs`](./docs) are automatically generated with [mdsplit](https://github.com/alandefreitas/mdsplit).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to go

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also this mkdocs warning:

[WARNING] Documentation file 'contributing.md' contains a link to 'README.md' which is not found in the documentation files.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be doing something wrong. But this is still there.

- Vectors: vectors.md
- Strings: strings.md
- Sets and Maps: sets_and_maps.md
- Reference: # Reference is organized by module
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure we want to use here is still a serious issue:

- Small: reference/Modules/index.md     

Refers to modules, but the page is empty because there are no doxygen modules.

- Index: reference/Namespaces/namespacesmall.md

Refers to the index. The index is including small::detail, which shouldn't be happening. We need to fix something in the Doxyfile.

It actually just shows a single symbol default_codepoint_hint_step in this namespace but I don't know why that is. At worst, we can always use #ifndef SMALL_DOXYGEN to remove it, but fixing the Doxyfile is ideal because it ensures this won't happen in the near future.

Also, the index is usually the last element in TOC because we only show everything there is after showing want we want users to see. But that's fine for now.

Something making the index ugly is the is_relocatable class and the fact that map is an alias, which gets mixed up with other less relevant aliases. We could solve this temporarily by including vector and basic_string directly in the TOC before the index. And maybe creating an exception to expose detail::associative_vector for now as the map interface, also in the TOC.

At last,

      - Classes: reference/Classes/index.md                  # All Classes
      - Files: reference/Files/index.md

are not very useful but I guess there's no harm in leaving them there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for the delayed response. For sure we need to figure this out. So the small::detail shouldn't appear in the documentation, right?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. 👍️

The detail namespace is always private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some changes and the namespace detail is not appearing anymore in the docs. Also, the Index section is the last element of TOC.

Last but not least, I didn't understand exactly the problem with is_relocatable. I added two Sections in the TOC with vector and basic_string, and I'm figuring out if it's possible to open an exception for detail::associative_vector. In the meanwhile, I'd to check with you if that makes sense.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some changes and the namespace detail is not appearing anymore in the docs.

Great. detail is only in the index now, when talking about sets and maps. But we do need a better solution for sets and maps anyway.

  • The best solution we could have for now (creating another issue) is to move the details that need to get exposed to another namespace, like exposition_only or something, so that Doxygen can render these for us. In the doxybook templates, we could include a big warning about this being exposition only whenever we render a class in this namespace. At least we would have something in the reference, because sets and maps are completely missing from the reference now.
  • Something that would also help, regardless, is to define the doxygen modules (creating another issue). The subsections of the reference should refer to modules and not classes because there are lots of free functions in the vector/string API and these are not rendered at the moment (to_string, stol, ...).
  • The best long-term solution, would be to completely refactor that when improving these associative containers, which needs to happen at some point.

I didn't understand exactly the problem with is_relocatable.

is_relocatable is now at a different position, so the previous comment doesn't completely apply anymore. It's now only listed in "Classes" and "Index". One problem now is that only the is_relocatable< small::basic_string< CharT, N, Traits, WCharT, Allocator, CP_HINT_STEP, SizeType > > specialization is appearing in the docs. I don't know why the main class is not in the docs, but I would say we should remove is_relocatable from the docs for now in that case. Because the list of classes as basic_string, is_relocatable< small::basic_string< CharT, N, Traits, WCharT, Allocator, CP_HINT_STEP, SizeType > >, and vector is not looking very good. We could find out why this is not rendering later and include it in this vector module.

I added two Sections in the TOC with vector and basic_string

Yes. That improved things a lot.

  • It's natural to call the string class/module only string instead of basic_string. basic_string is almost like an implementation detail. It's meant to be used by implementers to generalize string types and used rarely by very advanced users. For this reason, cppreference simply calls this the "string" library, which is what we can use in the TOC.
  • Since small::string depends on small::vector, it's natural that vector comes first in the TOC.

Also, I might be doing something wrong, but the very first subsection in the reference

- Small: reference/Modules/index.md        # futures module

is still completely empty: http://127.0.0.1:8000/small/reference/Modules/.

There's no point in a modules section since we have no modules (for now), let alone the very first section. There are also some references to "futures" in mkdocs.yml.

to open an exception for detail::associative_vector.

Including a detail in the public API, especially as the only class directly in the TOC is not a good idea because users are not supposed to use anything from the detail namespace. This is part of the contract. We could do the other exposition_only solution as a first step. We could do it in this PR or open another issue. I leave it up to you.

@@ -0,0 +1,10 @@
[data-md-color-scheme="small"] {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second look, the content tabs are not that bad.

@alandefreitas alandefreitas force-pushed the develop branch 2 times, most recently from d6340c6 to c605357 Compare May 19, 2022 17:57
@marcosfpr marcosfpr force-pushed the update-documentation branch from 6d1b74b to 5465f1f Compare June 10, 2022 13:27
@alandefreitas
Copy link
Owner

Nice. Besides the comments, one thing to notice is you have to rebase on top of develop again. I think the only difference now is that your other PR has been renamed. But you should keep rebasing anyway to ensure your branch is up to date.

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