fix: separate CLI launch description from info-collector description#10
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReworked launch description flow: the code no longer passes an initial description to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
zkraus
left a comment
There was a problem hiding this comment.
Suggested change for a discussion.
| @@ -158,15 +157,15 @@ def _process_test_suites(self, parser: JUnitParser, launch_name: str) -> Optiona | |||
| if promoted_props is not None: | |||
There was a problem hiding this comment.
I would propose a simplification. The end goal is to have
{cli_desc}
{separator}
{collector_desc}
While either of those is optional. I think we could simplify this as:
description_list = []
if promoted_description:
description_list.append(promoted_description)
if initial_launch_description:
description_list.append(initial_launch_description)
final_launch_description "\n\n".join(description_list)This way, both are ordered, optional, and separator is there if necessary. Also if we will find another source of description that we need to concatenate, it is easier to extend.
WDYT?
There was a problem hiding this comment.
Maybe we could rename initial_launch_description as cli_launch_description or cofiguration_lauch_descriptoin ? to express it is coming from cli or configuration
There was a problem hiding this comment.
I've implemented the changes you suggested. I just rearranged the order so that cli_description is first. I think it's better if you intentionally add a description there then you'll see it right at the beginning.
There was a problem hiding this comment.
Yes, thank you, I was not meaning to swap the order. You are correct.
6084c76 to
2be8c43
Compare
zkraus
left a comment
There was a problem hiding this comment.
Sorry to force another round of changes, looking at the simplified code now, I see another room for improvement.
Refactor redundant if statement with promoted_desc
Otherwise it looks great.
| @@ -158,15 +157,15 @@ def _process_test_suites(self, parser: JUnitParser, launch_name: str) -> Optiona | |||
| if promoted_props is not None: | |||
There was a problem hiding this comment.
Yes, thank you, I was not meaning to swap the order. You are correct.
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
2be8c43 to
8339ebb
Compare
Summary
--launch-descriptionwith__rp_launch_descriptionusing\n\nseparator instead of replacing one with the otherstart_launchto prevent RP client from concatenating start and finish descriptions without separationProblem
When
--launch-description "some description"was passed via pipeline, it appeared glued to the "Environment Information" heading with no line break:some description Environment Information (2 clusters)
Solution
\n\n+ info-collector description)finish_launch, not instart_launchResult
some description
Environment Information (2 clusters)
🤖 Generated with Claude Code
Summary by CodeRabbit