Skip to content

Conversation

@tisnik
Copy link
Contributor

@tisnik tisnik commented Sep 7, 2025

Description

LCORE-643: don't store None value in boolean CLI option

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Related Tickets & Documents

  • Related Issue #LCORE-643

Summary by CodeRabbit

  • Bug Fixes
    • The --verbose and --dump-configuration command-line options now default to off when not provided.
    • Values are consistently boolean, eliminating edge cases from None defaults and improving reliability in checks and automation.
    • Existing usage remains unchanged: pass the flag to enable.
    • No changes to public interfaces or other behaviors.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 7, 2025

Walkthrough

Updated argparse defaults in src/lightspeed_stack.py: the verbose and dump-configuration flags now default to False instead of None, keeping store_true flags consistently boolean. No other logic or public signatures changed.

Changes

Cohort / File(s) Summary
Argument parsing defaults
src/lightspeed_stack.py
Set default values for --verbose and --dump-configuration from None to False in create_argument_parser(), ensuring boolean outputs for store_true flags. No other logic modified.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

I twitch my ears at flags so bright,
From None to False—now set just right.
A boolean hop, a tidy trail,
No shifting sands, no cryptic veil.
With quiet paws, I test and gleam—
Verbose? Only when you deem. 🐇✨

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/lightspeed_stack.py (3)

28-35: Nit: default=False is redundant for store_true; also tighten help.

Argparse already defaults store_true to False. Consider dropping the explicit default and clarifying the help text.

         "-v",
         "--verbose",
         dest="verbose",
-        help="make it verbose",
+        help="Enable verbose logging",
         action="store_true",
-        default=False,
     )

36-43: Nit: same here—drop redundant default; clarify help.

         "-d",
         "--dump-configuration",
         dest="dump_configuration",
-        help="dump actual configuration into JSON file and quit",
+        help="Dump effective configuration to configuration.json and exit",
         action="store_true",
-        default=False,
     )

28-35: Optional: wire -v/--verbose to logging level.

If desired, bump log verbosity when the flag is set.

Add after Line 59 (right after args = parser.parse_args()):

if args.verbose:
    logging.getLogger().setLevel(logging.DEBUG)
    for h in logging.getLogger().handlers:
        h.setLevel(logging.DEBUG)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8565f47 and d0182a1.

📒 Files selected for processing (1)
  • src/lightspeed_stack.py (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build-pr
🔇 Additional comments (2)
src/lightspeed_stack.py (2)

28-35: Correct: store_true flags now stay boolean (no None).

This aligns with argparse semantics and keeps downstream checks like if args.dump_configuration: correct.

Also applies to: 36-43


28-35: Sanity check: ensure nothing relied on a None sentinel.

Search for ‘is None/!= None’ checks or Optional[bool] annotations on these flags to avoid behavior drift.

#!/bin/bash
set -euo pipefail
# Look for None-sentinel usage of CLI flags
rg -nP -C2 '\bargs\.(verbose|dump_configuration)\s+is\s+None'
rg -nP -C2 '\bargs\.(verbose|dump_configuration)\s+is\s+not\s+None'
rg -nP -C2 '\bargs\.(verbose|dump_configuration)\s*==\s*None'
rg -nP -C2 '\bargs\.(verbose|dump_configuration)\s*!=\s*None'
# Optional/typing clues
rg -nP -C2 'Optional\s*\[\s*bool\s*\].*\b(verbose|dump_configuration)\b'

Also applies to: 36-43

@tisnik tisnik merged commit 956969a into lightspeed-core:main Sep 7, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant