Adjust product_selector print format#694
Conversation
📝 WalkthroughWalkthroughAlright, quick one: ChangesProduct Selector String Formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Poem A rabbit hopped through selector code, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
21 fixed, 0 new since branch point (fc6b297) ✅ 21 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phlex/core/product_selector.cpp`:
- Around line 74-90: The creator formatting in product_selector.cpp does not
handle the stage-without-creator case, so the documented [ANY] fallback is
missing. Update the creator_str logic in the selector formatting path to detect
when stage.has_value() is true but creator is empty/falsy, and render [ANY]
instead of formatting an empty creator; keep the existing behavior for present
creators and no-stage cases. Also add a test in provider_test.cpp covering the
stage set with no creator path to verify the expected output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea160492-9125-45c6-948f-da3b5f8dbdf8
📒 Files selected for processing (2)
phlex/core/product_selector.cpptest/provider_test.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: build (gcc, none)
- GitHub Check: clang-tidy-check
⚠️ CI failures not shown inline (2)
GitHub Actions: beojan checking C++ code format / clang-format-check: beojan checking C++ code format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ clang-format check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::clang-format check failed. Please review the output above for details."�[0m
GitHub Actions: beojan checking C++ code format / 0_clang-format-check.txt: beojan checking C++ code format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ clang-format check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::clang-format check failed. Please review the output above for details."�[0m
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,cc,cxx,h,hpp}: Use clang-format tool for all C++ code formatting (VS Code auto-formats on save); configuration defined in.clang-formatwith 100-character line limit and 2-space indentation
Follow clang-tidy recommendations defined in.clang-tidy
Files:
test/provider_test.cppphlex/core/product_selector.cpp
**/*.{hpp,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,cpp}: Use.hppfor header files,.cppfor implementation, and*_test.cppfor test files in C++
Enforce 100-character line limit and 2-space indentation in C++ code via.clang-format
UseQualifierAlignment: Right(east-const) style:int const xnotconst int xin C++
UsePointerAlignment: Leftin C++ (pointer*attached to type, not variable name)
All C++ identifiers must uselower_casenaming: namespaces, classes, structs, enums, functions, variables, parameters, members, and constants
Exception to C++ naming: template parameters useCamelCase
Exception to C++ naming: macros useUPPER_CASE
Private, protected, and constant members in C++ must have a trailing underscore (_), no trailing underscore on anything else
Useenum classpreferred over plainenumin C++
Usestd::shared_ptrfor shared ownership,std::unique_ptrfor exclusive ownership, raw pointers for non-owning references only in C++
Use functors with agent-noun pattern:ModelEvaluator evaluate_model(...)in C++
Apply.clang-tidychecks for bugprone, cert, clang-analyzer, concurrency, cppcoreguidelines, misc, modernize, performance, portability, and readability as defined in the.clang-tidyconfiguration file
Usephlex::namespace for core code,phlex::experimental::for experimental features in C++
Files:
test/provider_test.cppphlex/core/product_selector.cpp
🔇 Additional comments (3)
phlex/core/product_selector.cpp (2)
80-83: LGTM!
77-79: 🎯 Functional CorrectnessNo change needed for
std::optional::transform.CMakePresets.jsonsetsCMAKE_CXX_STANDARDto 23, so this usage matches the project’s C++ dialect.> Likely an incorrect or invalid review comment.test/provider_test.cpp (1)
173-175: LGTM!Assertions match the new
to_string()composition, though as noted inproduct_selector.cpp, consider adding a case that covers stage-without-creator to actually exercise the[ANY]fallback the format comment promises.Also applies to: 197-198
Clang-Tidy Check ResultsFound 5762 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
2 participants
Addresses #677.
Will generate
[<suffix>][::<concept>][ by <creator>[ (of <stage>)]][ in <layer>]where square brackets indicate optional sections. If stage is specified but not creator,
creator will be
[ANY].Code
phlex::product_selector::to_string()to emit the new selector format:[<suffix>][::<concept>][ by <creator>[ (of <stage>)]][ in <layer>].[ANY].ϵ-based formatting path.Tests
test/provider_test.cppexpectations to match the revised product selector/error-message formatting in provider build/execute failure cases.