[Lint] Remove TODOs in sources

Closes OpenAssetIO#1091.

Issues created for TODOs that are still relevant:

- MacOS equivalent of `-Wl,--exclude-libs,ALL` OpenAssetIO#1439
- Making use of CMake's `FILE_SET` functionality OpenAssetIO#547.
- Customising the library name and namespace OpenAssetIO#1292.
- pylint scanning of extension modules OpenAssetIO#1136.
- Windows build improvements OpenAssetIO#538.
- Bump pybind11 version and remove LSan suppressions and bindings
  workarounds OpenAssetIO#1440.

TODO comments simply removed:

CI
- Linting of TraitGen isn't relevant to OpenAssetIO integration tests.

CMake
- There's not much we can/will do to resolve innocuous errors printed
  due to `MACOSX_RPATH TRUE`. Left a note explaining the issue.
- Nothing we can do about libabigail suppression `std::` regex bug, and
  using `--no-default-suppression` hasn't caused any problems so far.
  Left a note explaining the issue.
- We rejected an issue to investigate ASan on platforms other than Linux
  (OpenAssetIO#381), so no point leaving a TODO in the codebase to fix it for
  MacOS.
- Using `BUILD_SHARED_LIBS` is idiomatic, it's not clear what we'd do
  instead, and the TODO comment was written in the earliest days of
  OpenAssetIO whilst the CMake structure was still forming.
- The CMake `configure_package_config_file()`'s `PATH_VARS` argument is
  a way to notify CMake which variables are paths that need translating
  to fit the install tree. We don't make use of any such variables.
- Tested pybind11 version(s) is documented elsewhere, it is up to the
  consumer to choose an appropriate version. This is the same as for
  other dependencies. No need to pin the version in CMake.
- The location of `OPENASSETIO_CORE_ABI_VERSION` alongside other export
  related macros has turned out to be convenient, so no need to change
  this. There is an issue to extract macros to their own header (OpenAssetIO#1437),
  which may reconsider this.
- Changing the name of OpenAssetIO library file name(s) would be a
  breaking change, and the current names work just fine.
- Supporting pkg-config might be useful, but many other projects don't
  bother, and noone has requested this.

Conan
- More idiomatic/modern conanfile.py can/should be done as part of Conan
  2 adoption (OpenAssetIO#1313).
- libfmt latest versions no longer have the erroneous symbol export, so
  we're free to upgrade like any other library. However, we must
  consider downstream build pipelines before doing so (e.g. the latest
  libfmt version moves some symbols around).

FileUrlPathConverter
- There are no plans to diverge from compatibility with swift-url, so
  remove the TODOs on error priority. Leave explanatory notes.
- Ada supporting `percent_encode<true>` is off the table - left note in
  code comment to upstream issue.
- Optimisation of path parsing is unlikely to happen with the current
  implementation. The implementation is set to change by removing the
  PCRE2 dependency (OpenAssetIO#1289).

C API
- In general the C API is due a major re-think, so most TODOs just seem
  overly specific.
- There is no record of what "@exception messages" is supposed to mean.
- Granting access to tests to private sources is a common practice
  across the codebase.
- We're never likely to simulate and test a `std::bad_alloc` condition.

Python bindings/tests
- `isEntityReferenceString` tests now have a test for
  `kInfoKey_EntityReferencesMatchPrefix` so the TODO is defunct.
- There isn't much we can do to test/solve catastrophic `PyRetainingPtr`
  destruction corner cases. Leave notes explaining the potential issues.
- `__str__`/`__repr__` is now implemented, and tested in
  `test_printable.py`. The TODO re. "debug tricks" is ancient (predates
  the git repo), it's unclear what it means, but is almost definitely
  defunct.
- The pylint suppression of `abstract-method` is irrelevant since we
  migrated to C++, plus the mentioned issue OpenAssetIO#163 has been closed, so the
  TODO is defunct anyway.
- Suppressing the pylint `no-name-in-module` check for a couple of
  imports of `TraitsData` seems superfluous, and the comment(s) is
  ancient and seems to be regarding support for pylint in `--editable`
  installs, which won't be supported until OpenAssetIO#1309.
- Ancient TODO to assert that `TraitsData.traitSet` returns a `set()`
  that doesn't reference internals is unclear and/or seems superfluous,
  given that is how the pybind11 bindings work and is relied on in
  various other tests.

SimpleResolver
- The partial string test is sufficient. We could add an issue to update
  the test when OpenAssetIO/OpenAssetIO-Manager-BAL#127 is done, but
  it doesn't seem worth the effort.

Signed-off-by: David Feltell <david.feltell@foundry.com>

.github/workflows/integrations.yml

@@ -162,8 +162,6 @@ jobs:
         env:
           PYTHONPATH: ${{ github.workspace }}/dist/${{ matrix.config.site-packages }}
           CMAKE_PREFIX_PATH: ${{ github.workspace }}/dist
-          # TODO(DF): Consider adding some linting, e.g. compiler
-          # warnings, sanitizers. Will need additional CMake preset(s).
           OPENASSETIO_TRAITGENTEST_CMAKE_PRESET: test
           CONAN_USER_HOME: ~/conan
           # For Windows:

CMakeLists.txt

@@ -107,9 +107,6 @@ include(CMakeDependentOption)
 
 # Surface `BUILD_SHARED_LIBS` as a non-hidden CMake option in CMake
 # GUIs. This controls whether static or shared libs are built.
-# TODO(DF): We may end up needing to support building of both shared
-#   and static libs simultaneously. Also, using this means we inherit
-#   the option if openassetio is used as a CMake subproject.
 option(BUILD_SHARED_LIBS
     "Set to OFF to build static libraries. Static libaries are not recommended." ON)
 
@@ -315,7 +312,6 @@ configure_package_config_file(
     cmake/packaging/Config.cmake.in
     ${_project_config_file}
     INSTALL_DESTINATION ${_config_install_dir}
-    # TODO(DF): PATH_VARS?
 )
 
 install(
@@ -330,8 +326,6 @@ install(
     DESTINATION "${_config_install_dir}"
 )
 
-# TODO(DF): pkg-config? See OCIO.
-
 
 #-----------------------------------------------------------------------
 # Print a status dump

cmake/DefaultTargetProperties.cmake

@@ -68,7 +68,6 @@ function(openassetio_set_default_target_properties target_name)
 
     # Hide all symbols from external statically linked libraries.
     if (IS_GCC_OR_CLANG AND NOT APPLE)
-        # TODO(TC): Find a way to hide symbols on macOS
         target_link_options(${target_name} PRIVATE ${_exclude_all_libs_linker_flag})
     endif ()
 
@@ -119,10 +118,9 @@ function(openassetio_set_default_target_properties target_name)
             # Runtime search path value
             INSTALL_RPATH "${rpath}"
             # Enable RPATH on OSX
-            # TODO(TC): this produces seemingly innocuous
-            # install_name_tool errors during re-builds when it attempts
-            # to set the rpath in a previously installed target that
-            # hasn't changed.
+            # Note: this produces seemingly innocuous install_name_tool
+            # errors during re-builds when it attempts to set the rpath
+            # in a previously installed target that hasn't changed.
             MACOSX_RPATH TRUE
         )
     endif ()
@@ -263,7 +261,6 @@ endfunction()
 # This is disallowed by default in the above function
 # openassetio_set_default_target_properties.
 function(openassetio_allow_static_lib_symbol_export target_name)
-    # TODO(DF): only works on Linux - see above related TODO(TC).
     if (IS_GCC_OR_CLANG AND NOT APPLE)
         get_target_property(_link_options ${target_name} LINK_OPTIONS)
 

cmake/Testing.cmake

@@ -97,7 +97,7 @@ if (OPENASSETIO_ENABLE_TEST_ABI)
             # analysis" tree showing an example function that is
             # affected by the change.
             --leaf-changes-only
-            # TODO(DF): Bug in libabigail 2.4 `default.abignore` default
+            # Note: a bug in libabigail 2.5 `default.abignore` default
             # suppression file causes false negatives. Any ABI break
             # involving a function with `std::` in the demangled name is
             # suppressed - this includes template specialisations, e.g.
@@ -183,7 +183,6 @@ if (OPENASSETIO_ENABLE_PYTHON)
         # (`python` in this case) doesn't link libasan we must add it to
         # `LD_PRELOAD`. But first we have to find libasan on the system:
         execute_process(
-            # TODO(DF): This is probably wrong for OSX (clang).
             COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=libasan.so
             OUTPUT_VARIABLE asan_path
             OUTPUT_STRIP_TRAILING_WHITESPACE

cmake/ThirdParty.cmake

@@ -78,8 +78,6 @@ if (OPENASSETIO_ENABLE_PYTHON)
     endif ()
 
     # pybind11 for C++ Python bindings.
-    # TODO(DF): Our pybind11 conan package doesn't give version info, so
-    #   we can't pin it at the moment.
     find_package(pybind11 REQUIRED)
 
 

examples/host/simpleResolver/test_simpleResolver.py

@@ -66,7 +66,6 @@ def test_when_env_set_and_no_args_then_usage_printed_and_return_code_is_one(
             "simpleResolver.py: error: the following arguments are required: traitset, entityref"
         )
 
-        # TODO E.M, Revert this to being an exact check once BAL no longer prints "plugin" deprecation warning.
         assert expected_message in result.stderr
         assert result.returncode == 2
 
@@ -95,7 +94,6 @@ def test_when_entity_ref_invalid_then_error_and_return_code_wrapped(
     ):
         result = execute_cli("named", "bal:///doesNotExist")
         # Don't test the specific message as it couples to BAL specifics
-        # TODO E.M, Revert this to being a startswith check once BAL no longer prints "plugin" deprecation warning.
         assert "ERROR:" in result.stderr
         assert result.returncode == int(BatchElementError.ErrorCode.kEntityResolutionError)
 

resources/build/conanfile.py

@@ -7,8 +7,6 @@ class OpenAssetIOConan(ConanFile):
     # Generate a CMake toolchain preamble file `conan_paths.cmake`,
     # which augments package search paths with conan package
     # directories.
-    # TODO(DF): This is deprecated and should be swapped for the
-    # CMakeToolchain generator.
     generators = "cmake_paths"
 
     def generate(self):
@@ -47,9 +45,6 @@ def requirements(self):
         self.requires("catch2/2.13.8")
         # Mocking library
         self.requires("trompeloeil/42")
-        # TODO(DF): fmt v10 forcibly exports the symbol for its
-        #  `format_error` exception in GCC, making it not a true private
-        #  dependency. So pin to v9 for now.
         self.requires("fmt/9.1.0")
 
     def configure(self):

src/openassetio-core-c/CMakeLists.txt

@@ -8,7 +8,6 @@
 set(_public_header_source_root ${CMAKE_CURRENT_LIST_DIR}/include)
 
 # Installation location for install phase.
-# TODO(DF): When CMake 3.23 is released, use FILE_SET.
 install(
     DIRECTORY
     ${_public_header_source_root}/openassetio
@@ -27,7 +26,6 @@ add_library(${PROJECT_NAME}::openassetio-core-c ALIAS openassetio-core-c)
 # Set good default target options.
 openassetio_set_default_target_properties(openassetio-core-c)
 # Set output artifact base filename.
-# TODO(DF): Check other OSS projects how they name C vs C++ libs
 set_target_properties(openassetio-core-c PROPERTIES OUTPUT_NAME openassetio-c)
 # Add to the set of installable targets.
 install(TARGETS openassetio-core-c EXPORT ${PROJECT_NAME}_EXPORTED_TARGETS)

src/openassetio-core-c/include/openassetio/c/hostApi/Manager.h

@@ -67,14 +67,6 @@ typedef struct oa_hostApi_Manager_t* oa_hostApi_Manager_h;
  * to.
  *
  **/
-// TODO(DF): The ownership semantic of a
-// `ManagerInterface`/`HostSession` handle is "shared" (i.e. it wraps a
-// dynamically allocated `shared_ptr`), however there is no way to
-// "release" a `ManagerInterface` handle. We need to figure out where
-// these handles are created and what API we need around them - probably
-// a `dtor`, at minimum. Or possibly `Manager`s in the C API should only
-// be constructed via some factory, and there is no need for this `ctor`
-// and thus no need for a `ManagerInterface_h` handle?
 OPENASSETIO_CORE_C_EXPORT oa_ErrorCode
 oa_hostApi_Manager_ctor(oa_StringView* err, oa_hostApi_Manager_h* handle,
                         oa_managerApi_SharedManagerInterface_h managerInterfaceHandle,

src/openassetio-core-c/src/InfoDictionary.cpp

@@ -35,7 +35,6 @@ template <class... T>
  */
 template <typename Fn>
 oa_ErrorCode catchCommonExceptionAsCode(oa_StringView *err, const Fn &callable) {
-  // TODO(DF): @exception messages.
   return errors::catchUnknownExceptionAsCode(err, [&] {
     try {
       return callable();
@@ -66,7 +65,6 @@ oa_ErrorCode get(oa_StringView *err, Type *out, oa_InfoDictionary_h handle,
   const InfoDictionary *infoDictionary = handles::InfoDictionary::toInstance(handle);
 
   return catchCommonExceptionAsCode(err, [&] {
-    // TODO(DF): @exception messages.
     try {
       *out = std::get<Type>(infoDictionary->at({key.data, key.size}));
     } catch ([[maybe_unused]] const std::bad_variant_access &exc) {

src/openassetio-core-c/tests/CMakeLists.txt

@@ -40,8 +40,6 @@ target_include_directories(
     openassetio-core-c-test-exe
     PRIVATE
     # Give access to private headers.
-    # TODO(DF): This requires otherwise unnecessary symbol export, is
-    #   there a neater way?
     ${PROJECT_SOURCE_DIR}/src/openassetio-core-c/src
 )
 

src/openassetio-core-c/tests/InfoDictionaryTest.cpp

@@ -30,10 +30,6 @@ SCENARIO("InfoDictionary construction, conversion and destruction") {
   oa_StringView actualErrorMsg{errStorage.size(), errStorage.data(), 0};
 
   GIVEN("a InfoDictionary handle constructed using the C API") {
-    // TODO(DF): The only way InfoDictionary construction can error
-    //  currently is `bad_alloc` (i.e. insufficient memory), which is
-    //  a pain to simulate for testing.
-
     oa_InfoDictionary_h infoDictionaryHandle = nullptr;
     const oa_ErrorCode actualErrorCode =
         oa_InfoDictionary_ctor(&actualErrorMsg, &infoDictionaryHandle);
@@ -522,8 +518,6 @@ TEMPLATE_TEST_CASE_METHOD(MutatorFixture, "InfoDictionary mutated via C API", ""
     const openassetio::Str& nonExistentKeyStr = Fixture::kNonExistentKeyStr;
 
     // Storage for error messages coming from C API functions.
-    // TODO(DF): The only exception currently possible is `bad_alloc`,
-    //  which is tricky to test.
     openassetio::Str errStorage(kStrStorageCapacity, '\0');
     oa_StringView actualErrorMsg{errStorage.size(), errStorage.data(), 0};
 

src/openassetio-core/CMakeLists.txt

@@ -15,10 +15,6 @@ set(_core_abi_version 1)
 set(_public_header_source_root ${CMAKE_CURRENT_LIST_DIR}/include)
 
 # Installation location for install phase.
-# TODO(DF): When CMake 3.23 is released, use FILE_SET, which allows
-#   explicitly associating public headers with a target. Note that the
-#   PUBLIC_HEADER target property is not useful since it flattens the
-#   directory structure when installed.
 install(
     DIRECTORY
     ${_public_header_source_root}/openassetio
@@ -32,7 +28,6 @@ install(
 
 # Note: static vs. shared is auto-determined by CMake's built-in
 # BUILD_SHARED_LIBS option.
-# TODO(DF): Allow customising library name (e.g. suffix)? See OCIO.
 add_library(openassetio-core)
 add_library(${PROJECT_NAME}::openassetio-core ALIAS openassetio-core)
 # Set good default target options.
@@ -123,15 +118,9 @@ target_link_libraries(
 # API export header
 
 # Definition for export header, to use for versioned namespacing.
-# TODO(DF): It may turn out this should go in a separate header. Also
-#   other projects have much more elaborate version headers. See OCIO
-#   (bundles version in export header) and OTIO (uses separate header),
-#   both of which include a long list of additional #defines.
 set(_define_version
     "#define OPENASSETIO_CORE_ABI_VERSION v${_core_abi_version}")
 
-# TODO(DF): Allow customising namespace? See OCIO.
-
 # Use CMake utility to generate the export header.
 include(GenerateExportHeader)
 # Note: CMake>=3.30 recommended, since it adds Clang-Tidy linter

src/openassetio-core/src/utils/path/windows.cpp

@@ -58,8 +58,9 @@ Str FileUrlPathConverter::pathFromUrl(const std::string_view& url) const {
 
   Str decodedPath = ada::unicode::percent_decode(trimmedPath, trimmedPath.find(kPercent));
 
-  // TODO(DF): Ordering of validation to satisfy error priority of
-  // swift-url test cases.
+  // Note: validation is ordered to match swift-url's implementation,
+  // i.e. it satisfies the error priority of the test suite from the
+  // swift-url project, which we use for our unit tests.
 
   if (host.empty() && !driveLetterHandler.isAbsoluteDrivePath(decodedPath)) {
     throwError(kErrorRelativePath, url);

src/openassetio-core/src/utils/path/windows/detail.cpp

@@ -39,10 +39,10 @@ std::optional<Str> WindowsUrl::ip6ToValidHostname(const std::string_view& host)
 bool WindowsUrl::maybePercentEncodeAndAppendTo(const std::string_view& path, Str& appendTo) {
   // Ada will automatically %-encode upon setting the URL path, but
   // with a more limited set than we want.
-  // TODO(DF): Ideally we'd use `percent_encode<true>`, which appends
-  // to a given string if encoding is needed. However, that
-  // specialisation is not generated in ada v2.7.4 on MacOS/clang and
-  // fails to link. See https://github.com/ada-url/ada/issues/580
+  // Note: Cannot use `percent_encode<true>`, which appends to a given
+  // string only if encoding is needed. That specialisation is not
+  // generated in Ada on MacOS/clang and fails to link. Ada devs will
+  // not support it. See https://github.com/ada-url/ada/issues/580
   if (Str result;
       ada::unicode::percent_encode<false>(path, kPercentEncodeCharacterSet.data(), result)) {
     appendTo += result;

src/openassetio-core/src/utils/path/windows/pathTypes.cpp

@@ -29,7 +29,7 @@ void DrivePath::toUrl(const std::string_view& windowsPath, ada::url& url) const
 }
 
 void DrivePath::validatePath(const std::string_view& windowsPath) const {
-  // TODO(DF): Kludge to match error priority of swift-url. Otherwise
+  // Note: kludge to match error priority of swift-url. Otherwise
   // this would be handled by `isAbsoluteDrivePath`.
   if (detail::NormalisedPath::startsWithSlash(windowsPath)) {
     // Path starts with slash so is a relative path.
@@ -51,7 +51,6 @@ void DrivePath::setUrlPath(const std::string_view& windowsPath, ada::url& url) c
   // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay)
   assert(driveLetterHandler.isAbsoluteDrivePath(windowsPath));
 
-  // TODO(DF): This trimming logic could surely be optimised.
   const std::string_view trimmedPath = normalisedPathHandler.withoutTrailingDotsInFile(
       normalisedPathHandler.withoutTrailingDotsAsFile(
           normalisedPathHandler.withoutTrailingSpacesAndDots(

src/openassetio-python/bridge/CMakeLists.txt

@@ -7,10 +7,6 @@
 set(_public_header_source_root ${CMAKE_CURRENT_LIST_DIR}/include)
 
 # Installation location for install phase.
-# TODO(DF): When CMake 3.23 is released, use FILE_SET, which allows
-#   explicitly associating public headers with a target. Note that the
-#   PUBLIC_HEADER target property is not useful since it flattens the
-#   directory structure when installed.
 install(
     DIRECTORY
     ${_public_header_source_root}/openassetio
@@ -33,7 +29,6 @@ set_target_properties(openassetio-python-bridge PROPERTIES OUTPUT_NAME openasset
 # Add to the set of installable targets.
 install(TARGETS openassetio-python-bridge EXPORT ${PROJECT_NAME}_EXPORTED_TARGETS)
 
-# TODO(DF): This needs some thought to get right - see #538.
 if (WIN32 AND OPENASSETIO_ENABLE_PYTHON)
     # "TARGET_PDB_FILE is allowed only for targets with linker created
     # artifacts"

src/openassetio-python/cmodule/src/errors/exceptionsBinding.cpp

@@ -39,10 +39,7 @@ using openassetio::errors::UnhandledException;
  * pyModule.
  */
 template <class Exception>
-void setPyException(const Exception &exception,
-                    // TODO(DF): False positive in clang-tidy 12 :(
-                    // NOLINTNEXTLINE(readability-avoid-const-params-in-decls)
-                    const std::string_view pyClassName) {
+void setPyException(const Exception &exception, const std::string_view pyClassName) {
   // Python "raise from" logic. See pybind11::raise_from. We need this
   // to handle the case of a C++ exception being translated when the
   // Python error indicator is already set for a different exception.

src/openassetio-python/cmodule/src/hostApi/ManagerBinding.cpp

@@ -252,8 +252,8 @@ void registerManager(const py::module& mod) {
            py::call_guard<py::gil_scoped_release>{})
       .def(
           "entityTraits",
-          // TODO(DF): Technically we shouldn't need this overload,
-          // see similar comment for `resolve`.
+          // Note: Technically we shouldn't need this overload, see
+          // similar comment for `resolve`.
           [](Manager& self, const EntityReference& entityReference,
              const access::EntityTraitsAccess entityTraitsAccess, const ContextConstPtr& context) {
             return self.entityTraits(entityReference, entityTraitsAccess, context);
@@ -278,8 +278,8 @@ void registerManager(const py::module& mod) {
            py::call_guard<py::gil_scoped_release>{})
       .def(
           "entityTraits",
-          // TODO(DF): Technically we shouldn't need this overload,
-          // see similar comment for `resolve`.
+          // Note: Technically we shouldn't need this overload, see
+          // similar comment for `resolve`.
           [](Manager& self, const EntityReferences& entityReferences,
              const access::EntityTraitsAccess entityTraitsAccess, const ContextConstPtr& context) {
             return self.entityTraits(entityReferences, entityTraitsAccess, context);
@@ -316,11 +316,11 @@ void registerManager(const py::module& mod) {
            py::call_guard<py::gil_scoped_release>{})
       .def(
           "resolve",
-          // TODO(DF): Technically we shouldn't need this overload,
-          // since we can use a similar trick to C++ to default the
-          // appropriate overload's tag parameter, e.g.
+          // Note: Technically we shouldn't need this overload, since we
+          // can use a similar trick to C++ to default the appropriate
+          // overload's tag parameter, e.g.
           // `py::arg("errorPolicyTag") = {}`. However, this causes a
-          // memory leak in pybind11.
+          // memory leak in pybind11 v2.9.2.
           [](Manager& self, const EntityReference& entityReference,
              const trait::TraitSet& traitSet, const access::ResolveAccess resolveAccess,
              const ContextConstPtr& context) {
@@ -346,11 +346,8 @@ void registerManager(const py::module& mod) {
            py::call_guard<py::gil_scoped_release>{})
       .def(
           "resolve",
-          // TODO(DF): Technically we shouldn't need this overload,
-          // since we can use a similar trick to C++ to default the
-          // appropriate overload's tag parameter, e.g.
-          // `py::arg("errorPolicyTag") = {}`. However, this causes a
-          // memory leak in pybind11.
+          // Note: Technically we shouldn't need this overload, see
+          // similar comment for other `resolve` overload.
           [](Manager& self, const EntityReferences& entityReferences,
              const trait::TraitSet& traitSet, const access::ResolveAccess resolveAccess,
              const ContextConstPtr& context) {
@@ -368,13 +365,10 @@ void registerManager(const py::module& mod) {
            py::arg("pageSize"), py::arg("relationsAccess"), py::arg("context").none(false),
            py::arg("successCallback"), py::arg("errorCallback"),
            py::arg("resultTraitSet") = trait::TraitSet{}, py::call_guard<py::gil_scoped_release>{})
-      // TODO(DF): Technically we shouldn't need this overload,
-      // since we can use a similar trick to C++ to default the
-      // appropriate overload's tag parameter, e.g.
-      // `py::arg("errorPolicyTag") = {}`. However, this causes a
-      // memory leak in pybind11.
       .def(
           "getWithRelationship",
+          // Note: Technically we shouldn't need this overload, see
+          // similar comment for `resolve`.
           [](Manager& self, const EntityReference& entityReference,
              const trait::TraitsDataPtr& relationshipTraitsData, const std::size_t pageSize,
              const access::RelationsAccess relationsAccess, const ContextConstPtr& context,
@@ -454,13 +448,10 @@ void registerManager(const py::module& mod) {
           py::arg("relationsAccess"), py::arg("context").none(false), py::arg("successCallback"),
           py::arg("errorCallback"), py::arg("resultTraitSet") = trait::TraitSet{},
           py::call_guard<py::gil_scoped_release>{})
-      // TODO(DF): Technically we shouldn't need this overload,
-      // since we can use a similar trick to C++ to default the
-      // appropriate overload's tag parameter, e.g.
-      // `py::arg("errorPolicyTag") = {}`. However, this causes a
-      // memory leak in pybind11.
       .def(
           "getWithRelationships",
+          // Note: Technically we shouldn't need this overload, see
+          // similar comment for `resolve`.
           [](Manager& self, const EntityReference& entityReference,
              const trait::TraitsDatas& relationshipTraitsDatas, const std::size_t pageSize,
              const access::RelationsAccess relationsAccess, const ContextConstPtr& context,