Skip to content

refactor: modularize match, auto-complete, and loop execution handlers - #176

Open
bhagathkrishnacdac wants to merge 1 commit into
omec-project:mainfrom
bhagathkrishnacdac:bess-refactor-cli
Open

refactor: modularize match, auto-complete, and loop execution handlers#176
bhagathkrishnacdac wants to merge 1 commit into
omec-project:mainfrom
bhagathkrishnacdac:bess-refactor-cli

Conversation

@bhagathkrishnacdac

Copy link
Copy Markdown

Overview
This PR refactors the core CLI class to address cognitive complexity and deep nesting in several methods.

Key Changes

  • Initialization Refactoring: Moved terminal environment checks, file system setup, and interactive mode parsing into private helper methods (_setup_history_file, _setup_error_output, _setup_interactive_mode).
  • Decomposed Match Logic: Broke down the complex match loop into distinct phase helpers, separating keyword matching, variable matching, and trailing-input validation.
  • Simplified Completion Logic: Decomposed _do_complete into dedicated helpers for finding commands, calculating common prefixes, and printing matching outputs.
  • Segmented Command Loop: Separated interactive/file input retrieval from inner command execution within process_one_line to make exception handling and loop-termination steps safer and clearer.
  • Safeguarded Slice Access: Added validation checks in _process_variable_match to prevent IndexError on split evaluations of empty tokens.

Signed-off-by: bhagathkrishnacdac <bhagath.krishna@cdac.in>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Not ready to approve

Terminal error handling now prints to stdout and restore_echoctl() can raise when old_flags is unset, creating user-visible regressions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR refactors bessctl’s core CLI class to reduce cognitive complexity by decomposing initialization, command matching, auto-completion, and per-line execution into smaller helper methods.

Changes:

  • Extracted initialization responsibilities into _setup_history_file, _setup_error_output, and _setup_interactive_mode.
  • Decomposed match() and _do_complete() into phase-specific helpers to reduce nesting and clarify control flow.
  • Split process_one_line() into _read_input_line() and _execute_command(), and tightened history-file error handling.
File summaries
File Description
bessctl/cli.py Refactors CLI initialization, matching, completion, and command loop into helper methods; adjusts terminal/history error handling.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 4
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread bessctl/cli.py
Comment on lines +92 to +95
try:
return os.path.expanduser('~/.bess_history')
except:
return None
Comment thread bessctl/cli.py
Comment on lines +629 to +630
except Exception as e:
print(f"Unexpected terminal error: {e}")
Comment thread bessctl/cli.py
Comment on lines +634 to 638
import termios

cur_flags = termios.tcgetattr(sys.stdin)
new_flags = cur_flags
if self.old_flags[3] & termios.ECHOCTL:
Comment thread bessctl/cli.py
Comment on lines +645 to +646
except Exception as e:
print(f"Unexpected terminal error: {e}")

@gab-arrobo gab-arrobo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When trying to apply locally, I get lots of warnings due to empty trailing whitespaces

pr.patch:42: trailing whitespace.
    def __init__(self, cmdlist, fin=sys.stdin, fout=sys.stdout, ferr=None,
pr.patch:43: trailing whitespace.
                interactive=None, history_file=None):
pr.patch:44: trailing whitespace.
        self.cmdlist = cmdlist
pr.patch:45: trailing whitespace.
        self.fin = fin
pr.patch:46: trailing whitespace.
        self.fout = fout
warning: squelched 350 whitespace errors
warning: 355 lines add whitespace errors.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Not ready to approve

The updated terminal exception handlers print to stdout and use f-strings (breaking Python 2 parsing and risking corrupted CLI output), and initialization introduces a bare except: that should be narrowed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

bessctl/cli.py:95

  • Avoid bare except: here; it catches BaseException (e.g., KeyboardInterrupt/SystemExit) and can mask shutdown signals during CLI init. Narrow to Exception (or a specific OS/path exception) for safer behavior.
        try:  
            return os.path.expanduser('~/.bess_history')  
        except:  
            return None  

bessctl/cli.py:630

  • This exception handler prints to stdout and uses an f-string, which can pollute CLI output and also breaks Python 2 parsing (the rest of the file still has Python 2 compatibility paths). Prefer writing to self.ferr with % formatting (or format) instead.
        except Exception as e:
            print(f"Unexpected terminal error: {e}")

bessctl/cli.py:646

  • This exception handler prints to stdout and uses an f-string, which can pollute CLI output and also breaks Python 2 parsing (the rest of the file still has Python 2 compatibility paths). Prefer writing to self.ferr with % formatting (or format) instead.
        except Exception as e:
            print(f"Unexpected terminal error: {e}")
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

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.

3 participants