feat: Add support for configuration files via --config and .beautyshrc #262
Conversation
📝 WalkthroughWalkthroughAdded a --config CLI option and explicit config-file loading plus a new Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as BeautyshCLI
participant Config as config.py
participant EditorCfg as EditorConfig
participant Formatter
User->>CLI: run beautysh --config=custom.toml files...
CLI->>CLI: _extract_config_path(argv) -> config_file
CLI->>Config: load_config_from_file(custom.toml, strict=True)
alt file ok
Config-->>CLI: explicit_config
else error
Config-->>CLI: error -> stderr + exit(1)
end
CLI->>Config: load_config_from_beautyshrc()
Config-->>CLI: beautyshrc_config
CLI->>Config: load_config_from_pyproject()
Config-->>CLI: pyproject_config
CLI->>EditorCfg: load_editorconfig_settings()
EditorCfg-->>CLI: editorconfig_settings
CLI->>Config: merge_configs(editorconfig, pyproject, beautyshrc, explicit_config, cli_args)
Note over Config: Precedence → CLI args > explicit_config > .beautyshrc > pyproject > editorconfig
Config-->>CLI: merged_config
CLI->>Formatter: run formatter with merged_config (file I/O using UTF‑8)
Formatter-->>User: formatted output / errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
README.md (1)
70-85: Clarify the section name priority in the .beautyshrc example.The example shows
[beautysh]but based on the implementation inbeautysh/config.py(lines 42-51), the loader checks[tool.beautysh]first, then[beautysh], then root-level keys. Consider adding a note about this priority or showing both formats.Example addition:
The `.beautyshrc` file, located in the current working directory, is a configuration file read in TOML format. It can contain configuration options at the root level, or under the `[tool.beautysh]` or `[beautysh]` sections. -Example `.beautyshrc` content: +Example `.beautyshrc` content (using `[beautysh]` section): ```toml [beautysh] indent_size = 2 tab = true +``` + +Or using the `[tool.beautysh]` section (same as pyproject.toml): + +```toml +[tool.beautysh] +indent_size = 2 +tab = true</blockquote></details> <details> <summary>tests/unit/test_config_module.py (1)</summary><blockquote> `12-45`: **Add test coverage for the [beautysh] section.** The `TestLoadConfigFromFile` class tests `[tool.beautysh]` and root-level keys, but doesn't test the intermediate `[beautysh]` section. According to `beautysh/config.py` (lines 45-47), this is a supported fallback. Add this test case: ```python def test_loads_config_with_beautysh_section(self, tmp_path): config_file = tmp_path / ".beautyshrc" config_file.write_text( """ [beautysh] indent_size = 5 """ ) config = load_config_from_file(config_file) assert config["indent_size"] == 5beautysh/cli.py (2)
166-178: Consider extracting config path parsing to a helper method.The manual argument parsing logic is necessary to bootstrap config loading but could be more maintainable as a separate method.
Example refactor:
def _extract_config_path(self, argv: List[str]) -> Optional[str]: """Extract --config path from argv before full parsing. Args: argv: Command-line arguments Returns: Config file path if specified, None otherwise """ for i, arg in enumerate(argv): if arg == "--config": if i + 1 < len(argv): return argv[i + 1] break if arg.startswith("--config="): return arg.split("=", 1)[1] return NoneThen in
load_configuration:config_file_path = self._extract_config_path(argv) if config_file_path: self.config_file = config_file_path explicit_config_settings = load_config_from_file(Path(config_file_path))
155-155: Update comment to match actual priority order.The comment describes the priority order but uses
<symbols which might be ambiguous. Consider using explicit ordering that matches the docstring inbeautysh/config.py.Apply this diff:
- Priority: EditorConfig < pyproject.toml < .beautyshrc < --config file < CLI args + Priority (highest to lowest): CLI args > explicit config file (--config) > .beautyshrc > pyproject.toml > EditorConfig
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(3 hunks)beautysh/cli.py(7 hunks)beautysh/config.py(4 hunks)tests/unit/test_config_module.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/test_config_module.py (1)
beautysh/config.py (4)
load_config_from_beautyshrc(103-113)load_config_from_file(19-61)load_config_from_pyproject(64-100)merge_configs(163-208)
beautysh/cli.py (1)
beautysh/config.py (3)
load_config_from_beautyshrc(103-113)load_config_from_file(19-61)merge_configs(163-208)
🪛 Ruff (0.14.2)
beautysh/config.py
55-55: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (9)
README.md (2)
65-68: Documentation accurately reflects the new priority order.The updated configuration priority list correctly documents the new explicit config file and .beautyshrc sources, matching the implementation in
beautysh/config.py.
118-119: Well-documented CLI option.The
--configoption documentation clearly explains its purpose and override behavior.tests/unit/test_config_module.py (2)
89-119: Comprehensive test coverage for .beautyshrc loading.The tests properly validate both TOML section formats and missing file scenarios.
180-240: Excellent test coverage for the five-source merge priority.The test suite thoroughly validates:
- The correct priority order (CLI > explicit_config > beautyshrc > pyproject > editorconfig)
- None value filtering from CLI args
- Empty config handling
beautysh/cli.py (1)
81-87: Well-implemented --config argument.The argument definition correctly describes the override behavior and accepts a path parameter.
beautysh/config.py (4)
49-51: Verify root-level fallback behavior for unrelated keys.When no
[tool.beautysh]or[beautysh]section exists, the function assigns all root-level keys from the TOML file toconfig(line 51). This means unrelated keys likeversion,name, etc. would be returned as beautysh config. Is this intended?For example, if
.beautyshrccontains:name = "my-project" version = "1.0" indent_size = 2All three keys would be returned. Consider either:
- Documenting this as expected behavior for
.beautyshrcfiles- Or only returning known beautysh configuration keys
Based on the README (lines 72-73), it states "configuration options at the root level" are supported, so this appears to be intentional. However, you may want to add validation or filtering of known keys.
88-89: Good refactor for consistency.Changing from binary
open()toread_text()+loads()makes the pyproject loader consistent with the newload_config_from_filefunction, and works correctly with bothtomllibandtomli.
103-113: Well-designed delegation pattern.The
load_config_from_beautyshrcfunction properly delegates toload_config_from_file, avoiding code duplication.
163-208: Correctly implements the five-source merge priority.The
merge_configsfunction properly implements the documented priority order with None filtering for CLI args. The updated docstring and examples clearly explain the behavior.
|
Passing tests and a verification release if you wanted to test it. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(3 hunks)beautysh/cli.py(6 hunks)tests/unit/test_config_module.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (2)
beautysh/cli.py (1)
beautysh/config.py (3)
load_config_from_beautyshrc(103-113)load_config_from_file(19-61)merge_configs(163-208)
tests/unit/test_config_module.py (1)
beautysh/config.py (5)
load_config_from_beautyshrc(103-113)load_config_from_editorconfig(116-160)load_config_from_file(19-61)load_config_from_pyproject(64-100)merge_configs(163-208)
🪛 Ruff (0.14.2)
beautysh/cli.py
170-170: unindent does not match any outer indentation level
(invalid-syntax)
|
Everything is passing in my forked branch, so should pass on this next go. https://github.com/meek2100/beautysh/actions/runs/19005892891 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
beautysh/config.py (1)
19-62: The logger placement is actually correct.The past review comment's concern is not valid. The
logger.debugat line 54 will not execute whenconfigis empty because:
- If the file doesn't exist, the function returns at line 34 before reaching the logger
- If the file exists but produces an empty config, the
if config:check at line 53 evaluates toFalse(empty dict is falsy), preventing the logger from executingHowever, the static analysis hint about using an
elseblock is a valid style suggestion for clarity. This is completely optional:- if config: - logger.debug(f"Loaded configuration from {config_path}: {config}") - return config except OSError as e: logger.warning(f"Could not read {config_path}: {e}") return {} except tomllib.TOMLDecodeError as e: logger.warning(f"Could not parse {config_path}: {e}") return {} + else: + if config: + logger.debug(f"Loaded configuration from {config_path}: {config}") + return config
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
beautysh/config.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
beautysh/config.py
55-55: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (3)
beautysh/config.py (3)
88-89: LGTM! Improved compatibility with tomllib/tomli.Switching to text-based loading with
read_text()andtomllib.loads()ensures consistent behavior across both tomllib (Python 3.11+) and the tomli backport.
103-113: LGTM! Clean implementation.The function properly delegates to
load_config_from_file(), following the DRY principle and maintaining consistency with the existingload_config_from_pyproject()pattern.
163-208: LGTM! Correct priority order implementation.The merge logic correctly implements the five-source priority hierarchy:
- CLI arguments (highest)
- Explicit config file (--config)
- .beautyshrc
- pyproject.toml
- EditorConfig (lowest)
The CLI None-value filtering ensures that unset CLI options don't override valid config file settings, which is the correct behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
beautysh/config.py (1)
5-5: Critical:Dictis not imported but used throughout the file.The code uses
Dict[str, Any]in type annotations (lines 19, 43, 65, 92, 104, 117, 165-170) butDictis not imported fromtyping. This will cause aNameErrorat runtime.🐛 Proposed fix
-from typing import Any, Optional +from typing import Any, Dict, OptionalAlternatively, for Python 3.9+ consistency with line 138 and 212 which use lowercase
dict, you could replace allDictwithdictthroughout the file.
🧹 Nitpick comments (2)
beautysh/config.py (2)
82-86: Minor inconsistency: use.is_file()instead of.exists()for consistency.
load_config_from_fileuses.is_file()(line 32), but this function uses.exists(). While functionally equivalent for most cases,.is_file()is more precise and would correctly handle the edge case wherepyproject.tomlis a directory.♻️ Optional fix for consistency
- if not pyproject_path.exists(): + if not pyproject_path.is_file():
117-138: Inconsistent type hint style within the same function.Line 117 uses
Dict[str, Any](requires typing import) while line 138 usesdict[str, Any](builtin, Python 3.9+). Pick one style for consistency across the file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
beautysh/cli.pybeautysh/config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- beautysh/cli.py
🧰 Additional context used
🪛 Ruff (0.14.10)
beautysh/config.py
19-19: Undefined name Dict
(F821)
43-43: Undefined name Dict
(F821)
65-65: Undefined name Dict
(F821)
92-92: Undefined name Dict
(F821)
104-104: Undefined name Dict
(F821)
117-117: Undefined name Dict
(F821)
165-165: Undefined name Dict
(F821)
166-166: Undefined name Dict
(F821)
167-167: Undefined name Dict
(F821)
168-168: Undefined name Dict
(F821)
169-169: Undefined name Dict
(F821)
170-170: Undefined name Dict
(F821)
🔇 Additional comments (4)
beautysh/config.py (4)
19-62: Function logic is sound, but note the root-level fallback behavior.The implementation correctly prioritizes
[tool.beautysh]→[beautysh]→ root keys. However, the fallback to root-level keys (line 50-51) will include all TOML keys if no beautysh section is found. This is likely intentional for simple.beautyshrcfiles, but could unintentionally pick up unrelated keys if the wrong file is passed via--config.
104-114: LGTM!Clean delegation to
load_config_from_file, keeping the code DRY.
164-209: LGTM!The five-source merge logic correctly implements the documented priority order. The use of sequential
dict.update()ensures later sources override earlier ones, and filteringNonevalues from CLI args prevents unset arguments from clearing explicit config values.
212-243: No changes to review in this segment.This function appears unchanged in this PR.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @beautysh/cli.py:
- Line 9: The import line "from typing import Any, Optional, dict, list" is
invalid because dict and list are built-ins; update the import to use typing
generics or remove built-ins: either change it to "from typing import Any,
Optional, Dict, List" and update any type hints using dict/list to Dict/List, or
simply remove "dict, list" from the import if your code uses built-in dict/list
annotations (Python 3.9+), ensuring functions or annotations like those in this
module (e.g., any occurrences referencing dict or list types) are adjusted
accordingly.
In @beautysh/config.py:
- Line 5: The import line incorrectly tries to import the built-in name `dict`
from `typing`; remove `dict` from the import (keep `Any` and `Optional`) and use
the built-in `dict` in type annotations (or `typing.Dict` only if you need
Python <3.9 compatibility). Update the import statement near the top of the
module and adjust any annotations that referenced the imported `dict` to use the
built-in `dict` (or `Dict`) accordingly.
🧹 Nitpick comments (2)
beautysh/cli.py (1)
81-87: Consider whether config files should be able to specify other config files.The default value
config.get("config")allows a config file (like.beautyshrcorpyproject.toml) to specify another config file path. This could lead to unexpected behavior or circular references. Typically, the--configoption should only be settable via CLI.♻️ Suggested fix to prevent config file from specifying another config
parser.add_argument( "--config", type=str, - default=config.get("config"), + default=None, help="Path to a specific configuration file (e.g., .beautyshrc). " "Overrides auto-discovered config files.", )beautysh/config.py (1)
190-197: Docstring example output includes unexplained'tab': False.The example shows output
{'indent_size': 8, 'tab': False}but none of the input dictionaries include atabkey. Either update the example inputs to includetab, or remove it from the expected output.📝 Suggested fix
Example: >>> editorconfig = {"indent_size": 2} >>> pyproject = {"indent_size": 4} >>> beautyshrc = {"indent_size": 3} >>> explicit_config = {"indent_size": 6} >>> cli = {"indent_size": 8} >>> merge_configs(editorconfig, pyproject, beautyshrc, explicit_config, cli) - {'indent_size': 8, 'tab': False} + {'indent_size': 8}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
beautysh/cli.pybeautysh/config.py
🧰 Additional context used
🧬 Code graph analysis (1)
beautysh/cli.py (1)
beautysh/config.py (4)
load_config_from_beautyshrc(104-114)load_config_from_editorconfig(117-161)load_config_from_file(19-62)merge_configs(164-209)
🔇 Additional comments (8)
beautysh/cli.py (4)
11-17: LGTM!The new imports for
load_config_from_beautyshrcandload_config_from_filecorrectly align with the new configuration loading functions.
35-35: LGTM!The new
config_fileattribute is correctly typed and initialized to track the explicit configuration file path.
152-168: LGTM!The
_extract_config_pathmethod correctly handles both--config VALUEand--config=VALUEformats. The early exit when--configlacks a following value is appropriate.
170-214: LGTM with clarification.The implementation correctly loads configuration from all sources. CLI arguments take precedence via argparse's default value mechanism rather than through
merge_configs, which is a valid approach. The merged config becomes the defaults, and argparse handles any explicit CLI overrides.beautysh/config.py (4)
19-62: LGTM!The
load_config_from_filefunction is well-implemented with clear fallback logic ([tool.beautysh]→[beautysh]→ root-level), appropriate error handling, and informative logging at correct levels.
82-101: LGTM!The updates to
load_config_from_pyprojectalign with the newload_config_from_filepattern, using consistent text reading and error handling.
104-114: LGTM!Clean delegation to
load_config_from_file. Minor note: the docstring mentions[tool.beautysh]and root-level keys but omits the[beautysh]section fallback thatload_config_from_filealso supports.
199-209: LGTM!The merge implementation correctly applies configurations in priority order using
dict.update(). The None-filtering for CLI args prevents argparse defaults from overwriting explicit config file values.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
beautysh/config.py (1)
164-209: Docstring example output inconsistent with implementation.The example at line 197 shows
{'indent_size': 8, 'tab': False}as the expected output, but'tab': Falseis never set by any of the example inputs. The merge function only propagates values that exist in the input dictionaries—it doesn't inject default values.Suggested fix for the docstring
- >>> merge_configs(editorconfig, pyproject, beautyshrc, explicit_config, cli) - {'indent_size': 8, 'tab': False} + >>> merge_configs(editorconfig, pyproject, beautyshrc, explicit_config, cli) + {'indent_size': 8}
🤖 Fix all issues with AI agents
In @beautysh/cli.py:
- Around line 191-193: When a user passes config_file_path via --config the code
currently sets self.config_file and calls load_config_from_file but does not
surface missing-file errors; update the logic around
config_file_path/self.config_file and the call to load_config_from_file to
explicitly check that Path(config_file_path).exists() before loading, and if the
file is missing emit a warning or raise a user-visible error (use the CLI logger
or the existing logging facility) instead of silently proceeding with an empty
dict; ensure explicit_config_settings remains loaded only after the existence
check so the message references the provided config_file_path.
In @beautysh/config.py:
- Around line 19-62: The function load_config_from_file currently falls back to
using the entire parsed TOML data (variable data) as config when neither
"tool.beautysh" nor "beautysh" sections are present, which risks leaking
unrelated keys; update load_config_from_file so it does not blindly assign data
to config—either remove the root-level fallback entirely or whitelist/validate
keys from data against the expected beautysh config keys (e.g., allowed keys
list used to filter data) before returning; make the change where config is
assigned (the branches checking data.get("tool", {}).get("beautysh") and
data.get("beautysh")) and ensure the returned config only contains
known/validated keys or is empty if no valid keys exist, and keep the existing
logging behavior around logger.debug/logger.warning.
🧹 Nitpick comments (1)
beautysh/cli.py (1)
186-214: CLI args not passed to merge_configs, breaking documented priority.The docstring at lines 173-178 states CLI arguments have the highest priority, but
merge_configsis called without thecli_argsparameter (line 209-214). This means CLI arguments aren't part of the merge—they're applied later viaargparsedefaults increate_parser.While this works because
create_parseruses merged config as defaults, it's inconsistent with the documented API wheremerge_configsis supposed to handle all five sources. If a user callsmerge_configsdirectly expecting CLI args to be merged, they'd need to pass them explicitly.Consider either:
- Updating the docstring to clarify CLI args are handled by argparse separately, or
- Passing CLI args to
merge_configsfor consistency with the documented API# Merge configs return merge_configs( editorconfig_settings, pyproject_config, beautyshrc_config, explicit_config_settings, + # Note: CLI args are applied via argparse defaults in create_parser() )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
beautysh/cli.pybeautysh/config.py
🧰 Additional context used
🧬 Code graph analysis (1)
beautysh/cli.py (1)
beautysh/config.py (4)
load_config_from_beautyshrc(104-114)load_config_from_editorconfig(117-161)load_config_from_file(19-62)merge_configs(164-209)
🔇 Additional comments (6)
beautysh/config.py (2)
82-101: LGTM on pyproject.toml loading improvements.The changes to use
is_file()check andread_text()+loads()pattern are consistent with the newload_config_from_filefunction and provide better compatibility with bothtomllibandtomli.
104-114: LGTM on .beautyshrc loading.Clean delegation to
load_config_from_file. The docstring correctly documents that it checks for[tool.beautysh]or root-level keys.beautysh/cli.py (4)
35-35: LGTM on new config_file attribute.Storing the config file path for later access is a reasonable addition to the public API.
81-87: Default value from config may cause confusion.Line 84 sets
default=config.get("config"), which means the--configargument's default can be recursively loaded from a config file. This creates a potentially confusing scenario where a.beautyshrccould specify another config file to load. If this is intentional, consider documenting it; otherwise, the default should likely beNone.Is the recursive config specification intentional behavior? If not:
parser.add_argument( "--config", type=str, - default=config.get("config"), + default=None, help="Path to a specific configuration file (e.g., .beautyshrc). " "Overrides auto-discovered config files.", )
152-168: Manual argument parsing may conflict with argparse conventions.The
_extract_config_pathmethod manually parses--configbefore argparse runs. This works but doesn't handle:
- Short-form aliases if added later (e.g.,
-C)- Arguments after
--(which argparse treats as positional)This is acceptable for the current implementation, but be aware that
argv[i + 1]could return a flag (e.g.,--config --help) if the user makes a mistake—argparse would later catch this, so it's not a bug, just a minor UX consideration.
196-200: EditorConfig loaded only for the first non-flag file argument.The loop breaks after finding the first file argument (line 200). This means if multiple files with different
.editorconfigsettings are processed, only the first file's EditorConfig applies to all. This is existing behavior, but worth noting that it may not match user expectations for multi-file processing with different EditorConfig rules per file.Is this the intended behavior? For accurate per-file EditorConfig support, the config loading would need to happen per-file rather than once at startup.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @beautysh/config.py:
- Around line 100-110: The docstring for load_config_from_beautyshrc is
inaccurate about root-level keys; update the docstring to state that it
delegates to load_config_from_file and only looks for TOML sections
[tool.beautysh] or [beautysh] (not root-level keys) when loading configuration,
and mention the behavior change explicitly so callers understand
load_config_from_file is the authoritative parser.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
beautysh/cli.pybeautysh/config.py
🧰 Additional context used
🧬 Code graph analysis (1)
beautysh/cli.py (1)
beautysh/config.py (4)
load_config_from_beautyshrc(100-110)load_config_from_editorconfig(113-157)load_config_from_file(19-58)merge_configs(160-205)
🔇 Additional comments (8)
beautysh/config.py (3)
19-58: LGTM!The implementation correctly handles the configuration loading with proper priority (
[tool.beautysh]before[beautysh]), comprehensive error handling for both file I/O and TOML parsing, and appropriate fallback behavior. The fix for root-level fallback (mentioned in the commit message) properly prevents leaking unrelated TOML keys by only extracting from the expected sections.
80-86: LGTM!Good improvements: using
is_file()instead ofexists()is more precise, and theread_text() + loads()pattern is now consistent withload_config_from_file.
160-205: LGTM!The merge logic correctly implements the documented priority order. The update order (EditorConfig → pyproject → .beautyshrc → explicit config → CLI) ensures higher-priority sources override lower-priority ones. The debug log at line 204 is a helpful addition for troubleshooting configuration issues.
beautysh/cli.py (5)
8-17: LGTM!Imports are well-organized and the new configuration loading functions are correctly imported.
35-35: LGTM!The
config_fileattribute is appropriately typed and initialized.
81-87: LGTM!The
--configargument is well-defined with a clear help message explaining its precedence over auto-discovered config files.
152-168: LGTM!The pre-parsing logic correctly handles both
--config valueand--config=valueformats. The edge case where--config=(empty value) is provided will be caught later by theis_file()check, which is acceptable error handling.
170-230: LGTM!The configuration loading flow is well-structured:
- Pre-parses
--configbefore full argument parsing- Validates explicit config file existence and content with clear error messages
- Loads all configuration sources in proper order
- Merges configurations correctly
The design choice to use merged config values as argparse defaults (rather than passing CLI args to
merge_configs) works correctly—argparse naturally handles CLI argument precedence over default values.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @beautysh/config.py:
- Around line 106-116: The docstring for load_config_from_beautyshrc incorrectly
claims it checks for "root-level keys"; update the docstring to state that it
only looks for TOML sections [tool.beautysh] or [beautysh] (no root-level key
support) to match the actual behavior implemented by load_config_from_file and
the intended prevention of key leakage; reference load_config_from_beautyshrc
and load_config_from_file in the docstring change so callers understand the
exact lookup semantics.
🧹 Nitpick comments (1)
beautysh/cli.py (1)
344-349: Consider documenting the --config option in print_help.The
--configargument is defined but the customprint_helpmethod only documents the function styles. Users looking for configuration guidance might benefit from seeing config file precedence documented in the help output.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
beautysh/cli.pybeautysh/config.pytests/test_config.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_config.py (1)
beautysh/cli.py (2)
BeautyshCLI(26-377)load_configuration(170-230)
🪛 Ruff (0.14.11)
beautysh/config.py
34-34: Avoid specifying long messages outside the exception class
(TRY003)
58-58: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (12)
tests/test_config.py (4)
9-10: LGTM!The pytest import is appropriately added for the new test functions that use pytest fixtures (
tmp_path,capsys) andpytest.raises.
215-224: LGTM!Good test coverage for the happy path of loading configuration from an explicit
--configfile with[tool.beautysh]section.
227-240: LGTM!This test correctly validates the "no leak" behavior from the commit message — ensuring that root-level TOML keys are ignored and a missing
[beautysh]or[tool.beautysh]section results in an error exit.
243-256: LGTM!Solid test for error handling — verifies that invalid TOML syntax results in a clean exit with an informative error message to stderr.
beautysh/config.py (3)
19-64: LGTM!The function correctly implements the strict/non-strict behavior with proper error handling. The section lookup priority (
[tool.beautysh]then[beautysh]) is well-documented and implemented. The static analysis hints aboutTRY003are pedantic for this context — the error messages are appropriately descriptive.
86-103: LGTM!The refactor to use
read_text()+tomllib.loads()aligns with the pattern used inload_config_from_file, ensuring consistent TOML parsing behavior.
166-211: LGTM!The merge function correctly implements the documented priority order using sequential
dict.update()calls. The docstring, example, and implementation are all consistent.beautysh/cli.py (5)
8-17: LGTM!Imports are appropriately updated for the new configuration loading functionality.
35-35: LGTM!The
config_fileattribute appropriately stores the explicit config path for potential later use or inspection.
81-87: LGTM!The
--configargument is well-defined with clear help text explaining its purpose.
152-168: LGTM!The pre-parsing approach is necessary to load configuration before argparse processes arguments. Both
--config VALUEand--config=VALUEformats are correctly handled.
186-230: LGTM!The configuration loading logic is well-structured with comprehensive error handling. The strict validation for explicit
--configfiles ensures users get clear feedback when their config is invalid or missing the required section. The merge order correctly implements the documented priority.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
beautysh/cli.py (1)
280-282: Consider handling encoding errors gracefully.Enforcing UTF-8 is a reasonable default for consistency, but this is a breaking change for users with shell scripts in other encodings (e.g., Latin-1). A
UnicodeDecodeErrorwill propagate and produce a generic error message.Consider adding explicit error handling:
♻️ Suggested improvement
def read_file(self, filepath: str) -> str: # Enforce UTF-8 to ensure consistency across platforms - with open(filepath, encoding="utf-8") as f: - return f.read() + try: + with open(filepath, encoding="utf-8") as f: + return f.read() + except UnicodeDecodeError as e: + raise ValueError( + f"File '{filepath}' is not valid UTF-8. " + f"Please convert it to UTF-8 encoding: {e}" + ) from e
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdbeautysh/cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (1)
beautysh/cli.py (1)
beautysh/config.py (4)
load_config_from_beautyshrc(106-116)load_config_from_editorconfig(119-163)load_config_from_file(19-64)merge_configs(166-211)
🔇 Additional comments (6)
beautysh/cli.py (6)
8-17: LGTM!The new imports are correctly added for the explicit config file support and
.beautyshrcloading functionality.
35-35: LGTM!The new
config_fileattribute correctly stores the explicit config path for later reference.
81-87: LGTM!The
--configargument is well-defined. The early extraction via_extract_config_pathbefore full parsing is the correct approach since config values need to be loaded to set parser defaults.
152-168: LGTM!The extraction logic correctly handles both
--config valueand--config=valueformats. The early extraction before full parsing is necessary to load config defaults.Note: If
--configis provided as the last argument without a value, this returnsNonesilently. This is acceptable since argparse will handle the resulting behavior, but you may want to verify this edge case is tested.
186-230: LGTM!The configuration loading logic is well-structured:
- Strict mode for explicit config files ensures users get clear error messages for missing/invalid configs
- Error handling properly catches file I/O and TOML parsing errors
- The section existence check (lines 204-209) enforces that explicit configs must have meaningful content
- The merge order correctly implements the documented priority
The
cli_argsparameter is intentionally omitted frommerge_configssince CLI arguments will override these defaults through argparse.
291-292: LGTM!UTF-8 encoding with Unix-style line endings (
newline="\n") is the correct choice for shell scripts and ensures consistent output across platforms.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_integration.py (1)
96-108: LGTM!Good coverage of all three function style options. The loop-based approach works well here.
Consider using
pytest.mark.parametrizefor better test isolation and clearer failure reporting (you'd see which specific style failed rather than a generic test failure), but the current implementation is functional.beautysh/cli.py (1)
152-168: Edge case:--configwithout value silently returnsNone.If
--configis at the end of argv without a value, this returnsNonesilently (line 165). While argparse will eventually error on the malformed input, the user might be confused by a later cryptic argparse error rather than a clear "missing config path" message.Consider adding an early error for this case:
Proposed enhancement
if arg == "--config": if i + 1 < len(argv): return argv[i + 1] - break # Found --config but no value + sys.stderr.write("Error: --config requires a path argument\n") + sys.exit(1) if arg.startswith("--config="): - return arg.split("=", 1)[1] + value = arg.split("=", 1)[1] + if not value: + sys.stderr.write("Error: --config requires a path argument\n") + sys.exit(1) + return value
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
beautysh/cli.pytests/test_integration.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_integration.py (2)
tests/__init__.py (1)
assert_formatting(50-82)tests/conftest.py (1)
fixture_dir(9-11)
beautysh/cli.py (1)
beautysh/config.py (4)
load_config_from_beautyshrc(106-116)load_config_from_editorconfig(119-163)load_config_from_file(19-64)merge_configs(166-211)
🪛 Ruff (0.14.11)
beautysh/cli.py
285-288: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
tests/test_integration.py (2)
12-12: LGTM!Good addition of docstrings to existing test functions. They concisely describe each test's purpose without being verbose.
Also applies to: 17-17, 22-22, 27-27, 32-32, 37-37, 42-42, 47-47, 52-52, 57-57, 62-62, 67-67, 72-72, 77-77, 82-82, 87-87
91-93: LGTM!Simple test following the established pattern for testing special character handling in function names.
beautysh/cli.py (6)
8-17: LGTM!Imports are appropriate for the new configuration loading functionality.
35-35: LGTM!Appropriate addition to track the explicit config file path.
81-87: LGTM!Clear help text that explains both the purpose and the override behavior of the
--configoption.
186-230: LGTM!The configuration loading logic is well-structured with proper error handling:
- Strict mode correctly surfaces file I/O and parsing errors
- Missing section check provides clear user feedback
- Config sources are loaded in the correct order for merging
The
merge_configsfunction acceptscli_argsas an optional parameter, and omitting it here is correct since CLI arguments haven't been parsed yet—they'll override via argparse defaults.
280-288: LGTM on the UTF-8 enforcement.Good addition for cross-platform consistency. The error message is user-friendly and explains how to resolve the issue.
Regarding the static analysis hint (TRY003): The inline message is acceptable here since this is CLI-facing code where a helpful error message improves user experience. Creating a custom exception class for this single use case would be over-engineering.
297-298: LGTM!Good use of
newline="\n"to ensure consistent Unix-style line endings for shell scripts, and explicit UTF-8 encoding for cross-platform consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
beautysh/cli.py (1)
286-292: Consider extracting the error message or using a custom exception.The static analysis tool flags TRY003 for the long message in
ValueError. While the current implementation provides helpful user feedback, you could optionally define a custom exception class or use a constant for the message template.♻️ Optional: Use a custom exception or constant
+class FileEncodingError(ValueError): + """Raised when a file cannot be decoded as UTF-8.""" + pass def read_file(self, filepath: str) -> str: # Enforce UTF-8 to ensure consistency across platforms try: with open(filepath, encoding="utf-8") as f: return f.read() except UnicodeDecodeError as e: - raise ValueError( - f"File '{filepath}' is not valid UTF-8. Please convert it to UTF-8 encoding: {e}" - ) from e + raise FileEncodingError( + f"File '{filepath}' is not valid UTF-8: {e}" + ) from e
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
beautysh/cli.py
🧰 Additional context used
🧬 Code graph analysis (1)
beautysh/cli.py (1)
beautysh/config.py (4)
load_config_from_beautyshrc(106-116)load_config_from_editorconfig(119-163)load_config_from_file(19-64)merge_configs(166-211)
🪛 Ruff (0.14.11)
beautysh/cli.py
290-292: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
beautysh/cli.py (3)
152-173: LGTM!The manual pre-parsing approach is necessary to load configuration before creating parser defaults. Both
--config valueand--config=valueforms are handled correctly, and the error cases (missing or empty value) exit cleanly with informative messages.
175-235: LGTM!The configuration loading flow is well-structured:
- Explicit config (
--config) is loaded strictly with proper error handling- The empty config check (line 209) correctly identifies valid TOML without the required
[beautysh]or[tool.beautysh]section- CLI arguments override config values through argparse defaults rather than
merge_configs, which is the correct approach for this design
301-303: LGTM!Consistent UTF-8 encoding with
newline="\n"ensures shell scripts maintain Unix-style line endings across platforms.
|
@lovesegfault, |
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.