Add full_product_spec and use that in provider API#501
Add full_product_spec and use that in provider API#501beojan wants to merge 1 commit intoFramework-R-D:mainfrom
Conversation
For now product_query has a conversion operator so the tests don't have to be rewritten. This conversion operator has a commented out `deprecated` annotation (because we use `-Werror`).
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #501 +/- ##
=======================================
Coverage 85.57% 85.57%
=======================================
Files 142 143 +1
Lines 3604 3640 +36
Branches 616 619 +3
=======================================
+ Hits 3084 3115 +31
- Misses 310 317 +7
+ Partials 210 208 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| // Transitional automatic conversion operator so I don't have to rewrite all the tests | ||
| // The deprecated annotation is commented out because we have -Werror on | ||
| // [[deprecated( | ||
| // "Generation of a full_product_spec from a product_query is only transitionally supported")]] |
There was a problem hiding this comment.
The deprecation attribute is fine for our users. They decide how to build their own Phlex-dependent code, with or without -Werror and -Wno-error=deprecated-declarations.
For our own purposes, we'll continue building with -Werror and not exclude deprecated declarations. We should convert all tests to use the encouraged API. If we want to test the deprecation, we can add a dedicated compile-only test that verifies the deprecation message. I'm not sure that's necessary, though.
| namespace phlex::experimental { | ||
| class PHLEX_MODEL_EXPORT full_product_spec { | ||
| public: | ||
| full_product_spec(product_specification spec, identifier layer, identifier stage) : |
There was a problem hiding this comment.
I am leery of creating a full product specification when what is constructed is not, in fact, a full specification (the type is not set until later). We did this for product_query/product_specification, and I'd like to fix this...specifically, I'd like to avoid the need for setters wherever possible.
I think this means that the provider's output_product registration interface requires all of the data-product fields encapsulated by product_specification, layer, and stage, but maybe they can be received from the user in a more ergonomic way (e.g., require the arguments algorithm_name, suffix, layer, stage, where stage can default to an identifier that denotes the current process):
m.provide(...)
.output_product(
/* algorithm_name*/ creator,
/* identifier */ suffix,
/* identifier */ layer,
/* identifier, possibly defaulted */ stage
)How these fields are packaged is then an implementation detail.
|
@copilot resolve the merge conflicts in this pull request |
2 participants
For now product_query has a conversion operator so the tests don't have to be rewritten. This conversion operator has a commented out
deprecatedannotation (because we use-Werror).