-
Notifications
You must be signed in to change notification settings - Fork 52
Add --config option to specify explicit configuration path #563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@hukkin, |
KyleKing
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few minor comments as an active in the mdformat community and to possibly help this PR along, but I'm not a maintainer, so take my feedback with a grain of salt!
src/mdformat/_conf.py
Outdated
| with open(conf_path, "rb") as f: | ||
| try: | ||
| toml_opts = tomllib.load(f) | ||
| except tomllib.TOMLDecodeError as e: | ||
| raise InvalidConfError(f"Invalid TOML syntax: {e}") | ||
|
|
||
| _validate_keys(toml_opts, conf_path) | ||
| _validate_values(toml_opts, conf_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this snippet could be extracted into a shared helper? The only difference is the error message (which includes the config_path in the former)
src/mdformat/_conf.py
Outdated
| """ | ||
|
|
||
|
|
||
| def read_single_config_file(config_path: Path) -> tuple[Mapping, Path | None]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should be cached like read_toml_opts (not strictly necessary, see proposal to hoist outside of the for loop)
src/mdformat/_cli.py
Outdated
| except FileNotFoundError as e: | ||
| if config_override_path and str(config_override_path) == str(e.args[0]): | ||
| print_error(f"Configuration file not found at: {e.args[0]}") | ||
| return 1 | ||
| raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about raising a custom ConfigFileNotFoundError or ConfigOverrideNotFoundError (instead of the more general FileNotFoundError) with the relevant metadata, then this conditional logic isn't necessary?
src/mdformat/_cli.py
Outdated
| try: | ||
| toml_opts, toml_path = read_toml(path.parent if path else Path.cwd()) | ||
| if config_override_path: | ||
| toml_opts, toml_path = read_single_config_file(config_override_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually on caching, the config override file can be read outside of the loop because the configuration file isn't relative to the file path. Maybe something like:
(toml_opts, toml_path): tuple[...?, Path] = None, None
if config_override_path:
toml_opts, toml_path = read_single_config_file(config_override_path)
for path in file_paths:
...
This pull request introduces the
--config <path>command-line option, allowing users to explicitly define the path to their TOML configuration file, overriding the default recursive search for.mdformat.toml.This feature primarily supports integration with modern tooling like pre-commit hooks and centralized configuration management systems where config files may live outside the project root or in a custom directory.
Key Changes:
--config(typePath) tosrc/mdformat/_cli.py.read_single_config_fileinsrc/mdformat/_conf.pyto support direct path loading. Therunfunction logic prioritizes--configand correctly handles non-existent file paths by exiting with an error.test_config_override_precedence) verifying that the explicit--configcorrectly overrides auto-detected.mdformat.tomlsettings.docs/users/configuration_file.mdto document the new usage.InvalidPathexception class insrc/mdformat/_cli.pyto properly callsuper().__init__(path), resolving theflake8-bugbear(B042) warning.