fix: reformat groups.json and add --fix to linter#5638
Conversation
groups.json was reformatted with multi-line arrays by a recent merge, breaking the one-line-per-test convention documented in README.md. Changes: - Reformat groups.json to canonical format (one line per test, compact arrays, sorted keys, space before colon). - Add --fix mode to lint_groups_json.py that auto-reformats the file in-place, preventing this class of issue in the future.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe script now supports command-line arguments via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ 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 |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the lint_groups_json.py script by introducing an auto-fix feature via a new --fix command-line argument and refactoring the logic into modular functions. The changes include a new reformat_groups function to canonicalize the JSON format and updates to the linting logic to return error counts. Feedback was provided to improve the robustness of the reformat_groups function by adding error handling for file access, JSON parsing, and data type validation.
| def reformat_groups(groups_path): | ||
| """Read groups.json and rewrite it in the canonical one-line-per-entry format.""" | ||
| with open(groups_path, "r") as f: | ||
| data = json.load(f) | ||
|
|
||
| if not isinstance(data, dict): | ||
| print(f"ERROR: Top-level value must be a JSON object", file=sys.stderr) | ||
| return False | ||
|
|
||
| sorted_keys = sorted(data.keys()) | ||
| lines = ["{"] | ||
| for i, key in enumerate(sorted_keys): | ||
| groups = data[key] | ||
| # Sort groups for determinism, but keep special strings like @proxysql_min_version at end |
There was a problem hiding this comment.
The reformat_groups function lacks error handling for missing files or invalid JSON, and it doesn't verify that the values in the dictionary are lists of strings. This can lead to unhandled exceptions (e.g., FileNotFoundError, JSONDecodeError, or TypeError during sorting). Adding these checks ensures the script fails gracefully with informative error messages, consistent with the behavior of lint_groups.
def reformat_groups(groups_path):
"""Read groups.json and rewrite it in the canonical one-line-per-entry format."""
if not os.path.isfile(groups_path):
print(f"ERROR: {groups_path} not found", file=sys.stderr)
return False
try:
with open(groups_path, "r") as f:
data = json.load(f)
except json.JSONDecodeError as e:
print(f"ERROR: Invalid JSON in {groups_path}: {e}", file=sys.stderr)
return False
if not isinstance(data, dict):
print(f"ERROR: Top-level value must be a JSON object", file=sys.stderr)
return False
sorted_keys = sorted(data.keys())
lines = ["{"]
for i, key in enumerate(sorted_keys):
groups = data[key]
if not isinstance(groups, list) or not all(isinstance(g, str) for g in groups):
print(f"ERROR: Value for '{key}' must be a list of strings", file=sys.stderr)
return False
# Sort groups for determinism, but keep special strings like @proxysql_min_version at end1 participant
Summary
Recent merges (e.g., #5625) reformatted
groups.jsonwith multi-line arrays, breaking the one-line-per-test convention documented intest/tap/groups/README.md. This caused merge conflicts and CI lint failures on branches that added new entries in the correct format.Changes
test/tap/groups/groups.json: Reformat to canonical format — one line per test, compact arrays, sorted keys, space before colon.test/tap/groups/lint_groups_json.py: Add--fixmode that auto-reformats the file in-place. Usage:python3 test/tap/groups/lint_groups_json.py --fix. The linter now also prints a hint suggesting--fixwhen it finds errors.Test plan
python3 test/tap/groups/lint_groups_json.pypasses (385 entries, sorted, compact)python3 test/tap/groups/lint_groups_json.py --fixis idempotent (no-op on already-correct file)Summary by CodeRabbit
Release Notes
New Features
--fixcommand-line flag to automatically reformat and fix configuration files to canonical format with proper key ordering and consistent formatting.Bug Fixes