Work/1091 todos in src #22
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
feltech
force-pushed
the
work/1091-todosInSrc
branch
from
c163238 to
9ae07aa
Compare
Bumps [dawidd6/action-download-artifact](https://github.com/dawidd6/action-download-artifact) from 7 to 8. - [Release notes](https://github.com/dawidd6/action-download-artifact/releases) - [Commits](dawidd6/action-download-artifact@v7...v8) --- updated-dependencies: - dependency-name: dawidd6/action-download-artifact dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…b_actions/dawidd6/action-download-artifact-8 [CI]: Bump dawidd6/action-download-artifact from 7 to 8
Part of OpenAssetIO#1091. Previous versions of clang-tidy would incorrectly flag the `_1`, `_2`, etc placeholder macros from the Trompeloeil library as violating our naming convention. This no longer seems to be the case for clang-tidy v19. Signed-off-by: David Feltell <david.feltell@foundry.com>
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>
feltech
force-pushed
the
work/1091-todosInSrc
branch
from
9ae07aa to
ece892e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Closes # (issue)
Reviewer Notes
Test Instructions