Code review comment for lp:~thomas-voss/xorg-gtest/Doxygen

Revision history for this message
Chase Douglas (chasedouglas) wrote :

It's a good start, but it's missing quite a bit:

* There's no Doxyfile or additions to the Makefiles to create and install the documentation

* Each object should have a description of what it is, at least enough so people familiar enough with gtest can figure it out. For example, Environment (the class) should have a brief line stating that it is a global gtest environment, and then a full description of what the class does.

I think some of these issues will become apparent when Doxygen is run and you view the generated documentation. I don't think the documentation here would be enough for people to understand how to use it without looking at the headers.

Other items:

* Environment::SetUp and TearDown documentation should make it clear that the functions should only be called by subclasses of Environment. We don't want people to think they need to call the functions themselves, even if they should know that already :).

* The variadic Process::Start documentation is missing documentation for the variadic arguments. Its documentation is also indented too far.

* Test::SetUp used to assert, but I pushed a fix so it would throw an exception instead. gtest catches exceptions so they are handled as failures, so this should not change the default functionality. However, throwing an exception is consistent with other routines and allows the user to catch it themselves. In summary, the documentation needs to be fixed to state exception throwing instead of failure asserting.

* Test::TearDown documentation says: "Closes the display if not NULL." What could be NULL? I think it just needs to say "Closes the display connection."

* I think the "brief" command clutters the documentation in the sources. I would rather use the Doxygen option that assumes the first paragraph of a comment is the brief section, and the rest is the detailed section.

* I prefer not using abbreviations for words. Non-native speakers may not be able to understand "feat." or "c'tor".

* Documenting a constructor as "Constructor" is a bit repetitive :). A better brief description would be something like "Constructs an object to provide a global X server dummy environment". This may repeat other documentation elsewhere, but that's ok.

Thanks!

« Back to merge proposal