From c24398de7c92271f64bb9d949e0ebb6e64dcd921 Mon Sep 17 00:00:00 2001
From: Oliver Ruebel <oruebel@lbl.gov>
Date: Mon, 27 Jan 2025 14:57:26 -0800
Subject: [PATCH] Improve develeoper docs for REGISTER_SUBCLASS_WITH_TYPENAME

---
 docs/pages/devdocs/registered_types.dox | 72 ++++++++++++++++++++-----
 src/nwb/misc/AnnotationSeries.hpp       |  1 +
 2 files changed, 59 insertions(+), 14 deletions(-)

diff --git a/docs/pages/devdocs/registered_types.dox b/docs/pages/devdocs/registered_types.dox
index 1f01bc6d..89260a20 100644
--- a/docs/pages/devdocs/registered_types.dox
+++ b/docs/pages/devdocs/registered_types.dox
@@ -71,22 +71,12 @@
  * the name of the namespace in the schema (e.g., "core", "hdmf-common"). In this way
  * we can look up the corresponding class for an object in a file based on the
  * ``neurodata_type`` and ``namespace`` attributes stored in the file.
- *
- * \note
  * A special version of the ``REGISTER_SUBCLASS`` macro, called ``REGISTER_SUBCLASS_WITH_TYPENAME``,
  * allows setting the typename explicitly as a third argument. This is for the **special case**
- * where we want to implement a class for a modified type that does not have its
- * own `neurodata_type` in the NWB schema. An example is `ElectrodesTable` in NWB <v2.7, which
- * did not have an assigned `neurodata_type`, but was implemented as a regular
- * `DynamicTable`. To allow us to define a class `ElectrodeTable` to help with writing the table
- * we can then use ``REGISTER_SUBCLASS_WITH_TYPENAME(ElectrodeTable, "core", "DynamicTable")``
- * in the `ElectrodesTable` class. This ensures that the `neurodata_type` attribute is set
- * correctly to `DynamicTable` on write instead of `ElectrodesTable`. However, on read this
- * will by default not use the `ElectrodesTable` class but the regular `DynamicTable` class
- * since that is what the schema is indicating to use. In the registry, the class will still
- * be registered under the ``core::ElectrodesTable`` key, but with "DynamicTable" as the
- * typename value and the `ElectrodesTable.getTypeName` automatic override returning
- * the indicated typename instead of the classname.
+ * where the name of the class cannot be the same as the name of the type (e.g,. when implementing
+ * a class for schema that doesn't have an assigned type in the schema or a class that requires
+ * template parameters that are not part of the type name). See \ref using_registered_subclass_with_typename
+ * for details. 
  *
  * \note
  * ``DEFINE_FIELD`` and ``DEFINE_REGISTERED_FIELD`` create templated, non-virtual read 
@@ -240,5 +230,59 @@
  * DEFINE_REGISTERED_FIELD(getData, DynamicTable, "my_table", My data table)
  * @endcode
  *
+ * @section using_registered_subclass_with_typename Using REGISTER_SUBCLASS_WITH_TYPENAME
+ *
+ * The main use case for \ref REGISTER_SUBCLASS_WITH_TYPENAME is when we need to implement
+ * a class for type where the name of the class does not match the type name. E.g. using
+ * ``REGISTER_SUBCLASS_WITH_TYPENAME(ElectrodeTable, "core", "DynamicTable")``, 
+ * the class will be registered tn the registry,  under the ``core::ElectrodesTable`` key, 
+ * but with "DynamicTable" as the typename value and the `ElectrodesTable.getTypeName` 
+ * automatic override returning the indicated typename instead of the classname.
+ * The main use cases for this are to:
+ *
+ * * Implement a class for a modified type that does not have its  own `neurodata_type` 
+ *   in the NWB schema. An example is `ElectrodesTable` in NWB <v2.7, which
+ *   did not have an assigned `neurodata_type`, but was implemented as a regular
+ *   `DynamicTable`. To allow us to define a class `ElectrodeTable` to help with writing the table
+ *   we can then use ``REGISTER_SUBCLASS_WITH_TYPENAME(ElectrodeTable, "core", "DynamicTable")``
+ *   in the \ref AQNWB::NWB::ElectrodeTable "ElectrodeTable" class. This ensures that the `neurodata_type` attribute is set
+ *   correctly to `DynamicTable` on write instead of `ElectrodeTable`. 
+ * * Another example is where the class may require template parameters in C++. The 
+ *   template parameters are required in C++ but are not part of the `neurodata_type`. 
+ *   An example is \ref AQNWB::NWB::VectorData "VectorData" which uses a template parameter to define the 
+ *   data type of data that is manages. 
+ *
+ * @subsection limitations_registered_subclass_with_typename Limitations of REGISTER_SUBCLASS_WITH_TYPENAME
+ *
+ * The main limitaton of the  \ref REGISTER_SUBCLASS_WITH_TYPENAME approach is that on read, AqNWB 
+ * will use the default class associated with the `neurodata_type`. E.g., in the case of 
+ * the \ref AQNWB::NWB::ElectrodeTable "ElectrodeTable" class, by default the regular 
+ * \ref AQNWB::NWB::DynamicTable "DynamicTable" class will be used since that is what 
+ * the schema is indicating to use. Similarly, for \ref AQNWB::NWB::VectorData "VectorData" 
+ * the default `VectorData<std::any>` will be used on read. To support reading using the
+ * more specific types, we can use the \ref DEFINE_REGISTERED_FIELD macro to define
+ * read methods that will return the approbriate type, e.g.:
+ *
+ * \code
+ * DEFINE_REGISTERED_FIELD(readElectrodeTable,
+ *                         ElectrodeTable,
+ *                         ElectrodeTable::electrodeTablePath,
+ *                         "table with the extracellular electrodes")
+ * \endcode
+ *
+ * in \ref AQNWB::NWB::NWBFile "NWBFile" to read the \ref AQNWB::NWB::ElectrodeTable "ElectrodeTable",
+ * or 
+ *
+ * \code
+ * DEFINE_REGISTERED_FIELD(
+ *     readGroupNameColumn,
+ *     VectorData<std::string>,
+ *     "group_name",
+ *     "the name of the ElectrodeGroup this electrode is a part of")
+ * \endcode
+ *
+ * in the \ref AQNWB::NWB::ElectrodeTable "ElectrodeTable" to read the `group_name` column
+ * as `VectorData<std::string>` with the data type already specified as `std::string` at compile time.
+ *
  *
  */
\ No newline at end of file
diff --git a/src/nwb/misc/AnnotationSeries.hpp b/src/nwb/misc/AnnotationSeries.hpp
index a9ac2ac1..54fa28b1 100644
--- a/src/nwb/misc/AnnotationSeries.hpp
+++ b/src/nwb/misc/AnnotationSeries.hpp
@@ -34,6 +34,7 @@ class AnnotationSeries : public TimeSeries
   /**
    * @brief Initializes the AnnotationSeries
    * @param description The description of the AnnotationSeries.
+   * @param comments Comments about the AnnotationSeries.
    * @param dsetSize Initial size of the main dataset. This must be a vector
    *                 with one element specifying the length in time.
    * @param chunkSize Chunk size to use.