Conversation
|
@phlexbot format |
|
No automatic header-guards fixes were necessary. |
|
No automatic cmake-format fixes were necessary. |
|
No automatic markdownlint fixes were necessary. |
|
Automatic clang-format fixes pushed (commit 388095a). |
|
No automatic jsonnetfmt fixes were necessary. |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #379 +/- ##
==========================================
- Coverage 85.36% 82.98% -2.39%
==========================================
Files 122 127 +5
Lines 2433 3320 +887
Branches 389 587 +198
==========================================
+ Hits 2077 2755 +678
- Misses 233 354 +121
- Partials 123 211 +88
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 48 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
form/form_source.cpp
Outdated
| // product_query fields must be compile-time _id literals, not runtime strings. | ||
| // To add new products, add new s.provide() blocks below following the same pattern. | ||
|
|
||
| s.provide("provide_i", |
There was a problem hiding this comment.
Will this be needed for every data product we read from input? If so, we may want to think about a way to shorten or auto-generate these functions.
There was a problem hiding this comment.
This is already noted in the FIXME comment — it's a known limitation of Prototype 0.1 that will be addressed as the type list grows.
There was a problem hiding this comment.
Here too I wouldn't mind a git issue
|
@phlexbot format |
|
No automatic header-guards fixes were necessary. |
|
No automatic jsonnetfmt fixes were necessary. |
|
No automatic markdownlint fixes were necessary. |
wwuoneway
left a comment
wwuoneway
left a comment
There was a problem hiding this comment.
I have a structural concern regarding the separation between form_module and form_source, as both currently communicate directly with Phlex. form_source appears to focus on reading input files, while form_module likely handles the integration with Phlex. Should form_source be a standalone module that communicates directly with Phlex, or would it be more appropriate as an internal component used by form_module?
form/form_source.cpp
Outdated
| form::experimental::config::output_item_config input_cfg; | ||
| form::experimental::config::tech_setting_config tech_cfg; | ||
| input_cfg.addItem("i", input_file, technology); | ||
| input_cfg.addItem("j", input_file, technology); |
There was a problem hiding this comment.
Instead of debating what to hardcode ( here, placeholder i and j, or meaningful names), it would be preferable to make the code automatically detect available items from the input file. This would eliminate hardcoding entirely and make the module truly reusable and scalable. A real input file may contain many possible items, and production code should ideally adapt dynamically instead assuming a fixed set. I would suggest moving such hardcoded i and j to test code, while the production code should be intelligent enough to discover available items automatically.
There was a problem hiding this comment.
The current approach is simple and explicit for Prototype 0.1. Auto-discovery is the right long-term goal, but there are two concrete blockers that go beyond the scope of this PR:
product_queryfields require compile-time_idliterals and do not accept runtime strings, making it impossible to loop over a dynamically discovered list of products- FORM has no enumeration API to discover available items from an input file
I propose keeping the current approach for Prototype 0.1 and tracking auto-discovery as a follow-up issue once these blockers are resolved.
form/form_source.cpp
Outdated
| } | ||
| return *static_cast<int const*>(pb.data); | ||
| }) | ||
| .output_product(product_query{.creator = "input"_id, .layer = "event"_id, .suffix = "j"_id}); |
There was a problem hiding this comment.
The two provider lambdas for i and j are almost identical, with only the product name string differing. Consider extracting the common logic into a factory function or template to reduce code duplication and improve maintainability. This would also make it easier to add more products in the future.
There was a problem hiding this comment.
The duplication is not an oversight.
The lambda body duplication is minimal and intentional for clarity in Prototype 0.1
The output_product() call cannot be generalized due to product_query compile-time _id literal constraint — already noted in FIXME.
The strong reason to keep them separate is that Phlex itself makes this distinction at the framework level — I prefer to keep form_module and form_source separate. That said, what do you think @pcanal and @knoepfel ? |
|
Updated
This makes |
|
@phlexbot format |
|
The YAML file changes visible in this PR are from the Appears like @phlexbot format is auto-formatting the yaml files. |
Following up on my earlier concern: Chris confirmed that @phlexbot format had issues when this PR was first submitted, which were later fixed. Running the format step again this morning correctly updated the files that needed formatting (including the YAML files), which is why the PR now shows additional changes. |
|
If the current version passes all the checks, I'm fine with going with it for now and following up on the known issues in a later update. |
5 participants
Adds form_source.cpp implementing a FORM-based provider that retrieves data products from FORM storage into Phlex.
This is the source-side complement to form_module.cpp.