Skip to content

refactor: resolve duplicate string literals, mutable default arguments, and type mismatches in Python scripts - #174

Open
bhagathkrishnacdac wants to merge 1 commit into
omec-project:mainfrom
bhagathkrishnacdac:bess-code-quality-fix
Open

refactor: resolve duplicate string literals, mutable default arguments, and type mismatches in Python scripts#174
bhagathkrishnacdac wants to merge 1 commit into
omec-project:mainfrom
bhagathkrishnacdac:bess-code-quality-fix

Conversation

@bhagathkrishnacdac

Copy link
Copy Markdown

Overview
This PR addresses several critical code quality issues across the project's Python files (under bessctl/, bin/, and build.py), focusing on string deduplication, correct parameter initialization, and type safety.

Key Changes

  • Eliminated Mutable Default Arguments: Resolved a classic Python code smell in bessctl/measurement_utils.py by changing a mutable default parameter value to None and initializing it within the function block to prevent shared state bugs.
  • Consolidated Duplicate String Literals: Defined constants for repeated string literals across test suites, daemon management scripts, and devbind tools (e.g., duplicated log messages, interface formats, and testing MAC/URL strings).
  • Corrected Subprocess Argument Types: Resolved a typing mismatch in bessctl/commands.py where the argument passed to check_call did not match the expected type (e.g., passing a string instead of a list of arguments, or vice versa).
    Verification

These changes do not alter any external behaviors, console outputs, or build pipeline results. All changes represent standard, non-functional code quality improvements.

@bhagathkrishnacdac
bhagathkrishnacdac requested a review from a team July 27, 2026 04:10

@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.

Some minor changes due to warnings from git when applying patch. Also, some other minor format changes

$ git apply pr.patch
pr.patch:187: trailing whitespace.
        if rtt_percentiles is None:
pr.patch:234: trailing whitespace.
BLACKLISTED_HOST = 'www.blacklisted.com'
pr.patch:349: trailing whitespace.
UNBIND_OPEN_ERROR = "Error: unbind failed for %s - Cannot open %s"
pr.patch:413: trailing whitespace.
# constants for duplicate literals
warning: 4 lines add whitespace errors.

from pybess import protobuf_to_dict as pb_conv

# constants for duplicate literals
BLACKLISTED_HOST = 'www.blacklisted.com'

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.

Suggested change
BLACKLISTED_HOST = 'www.blacklisted.com'
BLACKLISTED_HOST = 'www.blacklisted.com'

Comment thread bin/dpdk-devbind.py
from os.path import exists, abspath, dirname, basename

# constants for duplicate literals
UNBIND_OPEN_ERROR = "Error: unbind failed for %s - Cannot open %s"

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.

Suggested change
UNBIND_OPEN_ERROR = "Error: unbind failed for %s - Cannot open %s"
UNBIND_OPEN_ERROR = "Error: unbind failed for %s - Cannot open %s"

Comment thread build.py
import textwrap
import argparse

# constants for duplicate literals

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.

Suggested change
# constants for duplicate literals
# constants for duplicate literals

will have RTT stats reported as the average of those seen between
subsequent calls to `next()`.
"""
if rtt_percentiles is None:

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.

Suggested change
if rtt_percentiles is None:
if rtt_percentiles is None:

Comment thread bin/dpdk-devbind.py
Comment on lines +407 to 408
print(UNBIND_OPEN_ERROR
% (dev_id, filename))

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.

Should this become a single line?

Suggested change
print(UNBIND_OPEN_ERROR
% (dev_id, filename))
print(UNBIND_OPEN_ERROR % (dev_id, filename))

Comment thread bin/dpdk-devbind.py
Comment on lines +512 to 513
print(UNBIND_OPEN_ERROR
% (dev_id, filename))

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.

Suggested change
print(UNBIND_OPEN_ERROR
% (dev_id, filename))
print(UNBIND_OPEN_ERROR % (dev_id, filename))

Comment thread bin/dpdk-devbind.py
Comment on lines +519 to 520
print(UNBIND_OPEN_ERROR
% (dev_id, filename))

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.

Suggested change
print(UNBIND_OPEN_ERROR
% (dev_id, filename))
print(UNBIND_OPEN_ERROR % (dev_id, filename))

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.

Pull request overview

This PR refactors several Python scripts/tests to reduce repeated string literals, fix a mutable default argument, and tighten up a subprocess invocation parameter type, aiming to improve code quality without changing intended behavior.

Changes:

  • Introduces constants to replace duplicated string literals across build tooling, CLI output, and module tests.
  • Fixes a mutable default argument in PortStatsGenerator by switching from list() to None + in-body initialization.
  • Adjusts subprocess.check_call(..., shell=...) to use a boolean True instead of the string 'True'.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
build.py Adds a constant for the builtin protobuf output directory and reuses it in generation/cleanup paths.
bin/dpdk-devbind.py Factors repeated error/help strings into constants and reuses them across error paths.
bessctl/test_samples.py Introduces a constant for the daemon start command string used across test setup/recovery.
bessctl/sugar.py Introduces a constant for the __bess_env__ prefix used in environment-variable substitution strings.
bessctl/module_tests/url_filter.py Replaces repeated host literals with constants in URL filter tests.
bessctl/module_tests/arp.py Replaces repeated MAC address literals with a constant in ARP responder tests.
bessctl/measurement_utils.py Fixes a mutable default argument by initializing rtt_percentiles safely per instance.
bessctl/commands.py Consolidates repeated CLI output strings into constants and fixes a shell parameter type.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread bessctl/commands.py
Comment on lines 1318 to +1322
cli.fout.write(' Available plugins: ')
if drivers:
cli.fout.write('%s\n' % ', '.join(plugins))
else:
cli.fout.write('(none)\n')
cli.fout.write(NONE_MESSAGE)
Comment thread bessctl/test_samples.py
bessctl = os.path.join(this_dir, 'bessctl')
sample_dir = os.path.join(this_dir, 'conf/samples')

# constants for duplicate lliterals
from pybess import protobuf_to_dict as pb_conv

# constants for duplicate literals
BLACKLISTED_HOST = 'www.blacklisted.com'
will have RTT stats reported as the average of those seen between
subsequent calls to `next()`.
"""
if rtt_percentiles is None:
Comment thread build.py
import textwrap
import argparse

# constants for duplicate literals
…s, and type mismatches in Python scripts

Signed-off-by: bhagathkrishnacdac <bhagath.krishna@cdac.in>
@gab-arrobo
gab-arrobo force-pushed the bess-code-quality-fix branch from e89998b to 86efcdd Compare August 7, 2026 18:58
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