Add metadata to component objects#596
Open
cmacmackin wants to merge 5 commits into
Open
Conversation
7a2c0df to
1e3cccb
Compare
Not completed for all components, which have dummy-data in the meantime.
As a nice side-effect, this also adds tests for instantiating all Components.
7c130ff to
27e6af8
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #596 +/- ##
==========================================
+ Coverage 49.09% 52.98% +3.88%
==========================================
Files 96 97 +1
Lines 10037 10078 +41
Branches 1452 1463 +11
==========================================
+ Hits 4928 5340 +412
+ Misses 4604 4215 -389
- Partials 505 523 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
Author
|
Irritatingly, the coverage test is failing because |
Collaborator
Author
|
@ZedThree I know you're busy but I thought you might be a good person to review this, as it involves some messing around with templates and meta-programming. |
This file contains hidden or 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
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.
Purpose
It could be difficult to produce meaningful error messages, because component names are not consistently stored and their type information is not available at runtime. This PR provide a standard interface for this metadata.
Change Summary
The name of the component object is now a mandatory parameter for the
Componentbase class. This gets stored in a private variable and can be accessed using the publicobjectName()method.The component type names were a bit more complicated, as it is useful to have these available both statically and dynamically. Components are now expected to provide a
static constexpr auto typecomponent which can be converted to a string. This is used by theRegisterComponenthelper type to set the type-name associated with the component class (as opposed to passing it to the constructor manually). Additionally, allComponentclasses must implement the virtual functiontypeName().For most sub-classes, this is done using the CRTP. Rather than inheriting directly from
Componentthey inherit fromtemplate<typname T> NamedComponent. The secondaryNamedComponentclass was required so that method implementations for theComponentbase class can still be put in a separate file from the header. Note that the reaction classes create many components from the same (possibly templated) class and therefore don't useNamedComponent. Instead they use custom logic to set the static constant and define the virtual method.A formatter has been defined for
Componentobjects, which makes use of theobjectName()andtypeName()methods. By default, this prints names with the formatobjname (typename), or justtypenameif these two names are the same.Validation
New unit tests were written for the
objectName()method and the formatter, which confirms that these work as intended.Additionally, a unit test was added which instantiates all component type registered with the factory and confirms that
typeName()returns the same name as used to create them with the factory. Note that this is the first time there has been any test that many of these components can be constructed at all.AI Assistance
No AI tools were used.
Documentation
The developer documentation has been updated to reflect the new requirements when writing a component class. I will leave it up to reviewers to advise whether they think any additional details would be beneficial.
Review Notes
This work was originally done on top of #428, but I've spun it off so it can be merged earlier.