refactor: decompose bind_var into domain-specific validation handlers - #179
Open
bhagathkrishnacdac wants to merge 3 commits into
Open
refactor: decompose bind_var into domain-specific validation handlers#179bhagathkrishnacdac wants to merge 3 commits into
bhagathkrishnacdac wants to merge 3 commits into
Conversation
Signed-off-by: bhagathkrishnacdac <bhagath.krishna@cdac.in>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors bind_var() in bessctl/commands.py by decomposing the prior monolithic validation/casting logic into domain-specific helper handlers and routing var types through a dispatch map, aiming to reduce cognitive complexity and improve maintainability.
Changes:
- Introduces dedicated handler functions for toggle/direction values, numeric parsing, collection parsing, string validation, and
map/pyobjevaluation. - Replaces the previous
if/eliftree insidebind_var()with ahandler_mapdispatch dictionary.
Suppressed comments (3)
bessctl/commands.py:565
VAR_TYPE_NAME_PLUSis not defined anywhere in the codebase (only referenced), so using it as a key will crash module import. This should be the literal'name+'var_type (as produced bysplit_var()).
'wid+': _handle_collections,
VAR_TYPE_NAME_PLUS: _handle_collections,
'opts': _handle_collections,
bessctl/commands.py:563
pause_workersis produced by the parser (var_type = 'pause_workers') but is missing fromhandler_map, so the new dispatch path skips validation/canonicalization entirely.
handler_map = {
'endis': _handle_endis_dir,
'dir': _handle_endis_dir,
'gate': _handle_numeric,
'socket': _handle_numeric,
'int': _handle_numeric,
'wid+': _handle_collections,
bessctl/commands.py:550
- The new
map/pyobjerror messages are less informative than before (lost examples and the originalmapformat guidance). Restoring the prior messages makes CLI failures much easier to diagnose.
except Exception as e:
msg = '"map" should be "key=val..."' if var_type == 'map' else \
'"pyobj" should be an object in python syntax'
raise cli.BindError(msg)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+524
to
+525
| if var_type == VAR_TYPE_NAME_PLUS: | ||
| return sorted(list(set(val.split()))) |
Comment on lines
+498
to
+506
| def _handle_endis_dir(cli, val, var_type): | ||
| if var_type == 'endis': | ||
| if 'enable'.startswith(val): | ||
| val = 'enable' | ||
| elif 'disable'.startswith(val): | ||
| val = 'disable' | ||
| else: | ||
| raise cli.BindError('"endis" must be either "enable" or "disable"') | ||
|
|
||
| elif var_type == 'dir': | ||
| if 'in'.startswith(val): | ||
| val = 'in' | ||
| elif 'out'.startswith(val): | ||
| val = 'out' | ||
| else: | ||
| raise cli.BindError('"dir" must be either "in" or "out"') | ||
|
|
||
| elif var_type == 'wid+': | ||
| val = [] | ||
| for wid_str in head.split(): | ||
| if wid_str.isdigit(): | ||
| val.append(int(wid_str)) | ||
| else: | ||
| raise cli.BindError('"wid" must be a positive number') | ||
| val = sorted(list(set(val))) | ||
|
|
||
| elif var_type == 'host': | ||
| if 'enable'.startswith(val): return 'enable' | ||
| if 'disable'.startswith(val): return 'disable' | ||
| raise cli.BindError('"endis" must be either "enable" or "disable"') | ||
| if var_type == 'dir': | ||
| if 'in'.startswith(val): return 'in' | ||
| if 'out'.startswith(val): return 'out' | ||
| raise cli.BindError('"dir" must be either "in" or "out"') |
Comment on lines
+535
to
+539
| elif var_type == 'name' and re.match(r'^[\S]*$', val) is None: | ||
| raise cli.BindError('"name" must not contain whitespaces') | ||
| elif var_type in ['confname', 'filename'] and '\0' in val: | ||
| raise cli.BindError(f'Invalid {var_type}') | ||
| return val |
Comment on lines
+541
to
+546
| def _handle_eval(cli, val, var_type): | ||
| try: | ||
| if var_type == 'map': | ||
| return eval('_parse_map(%s)' % val) | ||
| # pyobj case | ||
| return eval(val) if val.strip() != '' else None |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR refactors bind_var to improve maintainability and resolve cognitive complexity. The monolithic if/elif conditional tree used to validate and cast CLI inputs has been replaced with modular, focused parser functions.
Key Changes
Modular Input Handlers: Created dedicated private helper functions categorized by validation domain: