Refactor generators and improve configuration handling #23
Conversation
- Introduce `region` field to markdown generator configuration for injecting specific regions in documentation.
- Validate `region` functionality and enforce `regions` optional dependency if used.
- Update `.pre-commit-config.yaml` and `pyproject.toml` with `email` and `regions` extra dependencies.
- Add `InjectedConfiguration.md` as an example of region-based documentation generation.
- Include new dependencies (`email-validator`, `text-region-parser`) in `uv.lock`.
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pydantic_settings_export/settings.py (1)
23-31: 💡 Verification agent🧩 Analysis chain
Verify the impact of removed generator configuration.
The removal of generator-related fields from
PSESettingsaligns with the refactoring goals. However, ensure that all existing usages of the removed functionality have been updated accordingly.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the removed generator configuration rg -A 5 "generators\s*=|generators_list\s*=|env_file\s*=" --type pyLength of output: 1872
Verify Generator Configuration Impact Across the Codebase
The refactoring removing generator-related fields in
PSESettingsaligns with the intended modular design. However, note the following concerns:
- In pydantic_settings_export/cli.py, there are still assignments using generator configuration (e.g.,
s.generators_list = generators). Ensure that these references are updated to reflect the new design or handled appropriately if they now rely on a different mechanism.- The utilities in pydantic_settings_export/utils.py invoke
AbstractGenerator.generators(), which appears independent ofPSESettingschanges. However, verify that any interaction with generator settings in CLI does not lead to runtime errors due to missing fields.Please confirm that all usages of the now-removed generator-related functionality have been refactored accordingly to avoid inconsistencies or failures.
🧹 Nitpick comments (10)
pydantic_settings_export/utils.py (1)
19-45: Consider handling row/column length mismatches.
When iterating over rows, there's no explicit check that each row matches the length ofheaders. If a row is shorter or longer, an index error could occur. For safety, you could add a length check before processing each row.def make_pretty_md_table(headers: list[str], rows: list[list[str]]) -> str: # noqa: C901 ... + # Validate that each row has the same number of cells as headers + for row_index, row in enumerate(rows): + if len(row) != len(headers): + raise ValueError( + f"Row {row_index} length ({len(row)}) does not match headers length ({len(headers)})." + ) col_sizes = [len(h) for h in headers] ...pydantic_settings_export/generators/abstract.py (2)
49-55: Add a docstring for thegenerator_configparameter.The constructor gains a new parameter
generator_config, but the docstring does not mention it. Documenting the purpose and usage of this parameter would improve readability.
66-66: Guard againstNonedocstrings.Appending a string to
cls.config.__doc__could raise an error ifcls.config.__doc__isNone. Consider a fallback assignment, for example:cls.config.__doc__ = (cls.config.__doc__ or "") + f"\n\nGenerator name: `{cls.name}`."- cls.config.__doc__ += f"\n\nGenerator name: `{cls.name}`." + cls.config.__doc__ = (cls.config.__doc__ or "") + f"\n\nGenerator name: `{cls.name}`."pydantic_settings_export/cli.py (1)
111-121: Gathering generator instances from config.Iterating over each named generator config to instantiate them is an excellent pattern. It cleanly finishes the pipeline from config -> generator objects.
Consider logging a warning if a generator name does not exist in
all_generatorsfor better debugging.pydantic_settings_export/models.py (3)
1-2: Consolidate import statements if necessary.
Imports forjson,typing, andpydantichave been added. Ensure all imports are actually needed. If certain imports are unused, consider removing them to keep the file clean.Also applies to: 8-8
126-129: Revisit example-check logic.
has_exampleschecksself.examples != [self.default]. Ifdefaultis a string or if there are multiple examples, this might skip certain corner cases (e.g., multiple examples, some matching the default). Consider whether you need stricter or more flexible logic here.
188-198: Avoid duplicate aliases.
When gathering aliases fromfield.alias,field.validation_alias,AliasChoices, orAliasPath, consider deduplicating them. Repeated entries can lead to confusion or inconsistent usage down the line.pydantic_settings_export/exporter.py (1)
5-6: Refine generator handling approach.
Switching from a list of generator types to a list of generator instances is cleaner. If you still want a default set of generators when none is provided, consider adding a fallback instead of storingNone. Also confirm that removingself.settingsfrom the generator’sruncall does not break existing usage patterns.Also applies to: 18-21, 37-37
pydantic_settings_export/generators/dotenv.py (1)
75-79: Consider extracting mode mapping to a constant.The mode mapping dictionary could be moved to a class-level constant for better readability and reusability.
+MODE_SETTINGS = { + "all": (True, True), + "only-optional": (True, False), + "only-required": (False, True), +} def generate_single(self, settings_info: SettingsInfoModel, level=1) -> str: result = "" - is_optional, is_required = { - "all": (True, True), - "only-optional": (True, False), - "only-required": (False, True), - }.get(self.generator_config.mode, (True, True)) + is_optional, is_required = self.MODE_SETTINGS.get(self.generator_config.mode, (True, True))README.md (1)
29-29: Improve Language Clarity
The sentence “This project is not well-designed for using as a library. But you still can use it as a code.” can be rephrased for better clarity and flow. Consider a revision such as: “This project is not primarily designed as a library, but it can still be used as standalone code.”🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: These words/punctuation marks might seem a little out of order. For clarity and coherence, try switching them around.
Context: ...esigned for using as a library. But you still can use it as a code. ```python from pydan...(AI_EN_LECTOR_REPLACEMENT_WORD_ORDER)
🛑 Comments failed to post (1)
pydantic_settings_export/models.py (1)
50-80:
⚠️ Potential issueEnsure consistent list return for
ForwardRefannotations.
Inget_type_by_annotation, when encountering atyping.ForwardRef, the code returns a string instead of a list. This contradicts the function signature-> list[str]. Consider wrapping the return in a list for consistency, e.g.:-if isinstance(annotation, typing.ForwardRef): - return annotation.__forward_value__ or annotation.__forward_arg__ +if isinstance(annotation, typing.ForwardRef): + return [annotation.__forward_value__ or annotation.__forward_arg__]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def get_type_by_annotation(annotation: Any, remove_none: bool = True) -> list[str]: args = getattr(annotation, "__args__", []) if remove_none: args = [arg for arg in args if arg is not None] # If it is an Alias (like `list[int]`), get the origin (like `list`) if isinstance(annotation, typing.GenericAlias): annotation = annotation.__origin__ # If it is a Union, get all types to return something like `integer | string` # instead of `Union[int, str]`, `int | str`, or `Union`. if isinstance(annotation, UnionType | typing._UnionGenericAlias): args = list(filter(bool, args)) if args: return [t for arg in args for t in get_type_by_annotation(arg)] else: annotation = None # If it is a Literal, get all types to return in "original" format like `1 | 'some-str'` # instead of `Literal[1, 'some-str']` or `Literal`. if isinstance(annotation, typing._LiteralGenericAlias): return [json.dumps(a, default=repr) for a in args] # If it is a ForwardRef, get the value or the argument to return something like `CustomType` # instead of `"CustomType"`, `ForwardRef("CustomType")` or `ForwardRef`. if isinstance(annotation, typing.ForwardRef): return [annotation.__forward_value__ or annotation.__forward_arg__] # Map the annotation to the type in the FIELD_TYPE_MAP return [FIELD_TYPE_MAP.get(annotation, annotation.__name__ if annotation else "any")]
📝 WalkthroughWalkthroughThe pull request introduces updates to configuration settings, documentation, and code to enhance region-specific markdown processing. The pre-commit hook configuration now specifies additional dependencies, while optional dependency groups for email and regions are added in the project configuration. Documentation is improved with an example illustrating how injected configuration regions function. In the codebase, a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Generator Requester
participant MGen as MarkdownGenerator
participant Parser as text_region_parser
participant FS as File System
Caller->>MGen: run(settings_info)
alt region not set
MGen->>MGen: Call default run behavior (superclass)
else region set
MGen->>Parser: Import and generate region-specific content
Parser-->>MGen: Return parsed region content
MGen->>FS: Check file existence and update content if needed
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pydantic_settings_export/generators/markdown.py (2)
88-96: Clarify documentation forregion
The docstring references "IfTrue, ..." even thoughTrueis not a valid literal forregion. Consider updating the description or the type hint to avoid confusion.- "If `True`, will be generated with the header of top-level settings.\n\n" + "If a string is provided, the generator will insert content into that named region.\n\n"
227-265: Region-based content replacement
If the same region placeholder appears multiple times, all instances will be replaced with the same content. This may be helpful or unexpected. Documenting this behavior could prevent confusion for end-users.docs/InjectedConfiguration.md (1)
16-16: Refine explanatory text
The phrase “Useful, then you have a Settings class/instance...” is slightly unclear. A small grammar fix would improve clarity.-| `PYDANTIC_SETTINGS_EXPORT__ENV_FILE` | `Path` \| `NoneType` | `null` | The path to the `.env` file to load environment variables. Useful, then you have a Settings class/instance, which require values while running. | `null` | +| `PYDANTIC_SETTINGS_EXPORT__ENV_FILE` | `Path` \| `NoneType` | `null` | The path to the `.env` file to load environment variables. Useful when you have a Settings class/instance that requires values while running. | `null` |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.pre-commit-config.yaml(1 hunks)docs/InjectedConfiguration.md(1 hunks)pydantic_settings_export/generators/markdown.py(4 hunks)pyproject.toml(2 hunks)
🔇 Additional comments (6)
pydantic_settings_export/generators/markdown.py (3)
1-1: Add import statement looks fine
This import is used properly to check for optional module availability.
5-5: Confirm Python version support for 'Self'
TheSelftype requires at least Python 3.11. Please verify the project's minimum version requirement is set accordingly to avoid compatibility issues in older environments.
114-122: Region validation logic
The check fortext_region_parseris well-implemented and straightforward..pre-commit-config.yaml (1)
67-69: Additional dependencies for the hook
Adding[email,regions]aligns with the new regional injection feature. The change appears correct and consistent with the associated code.pyproject.toml (2)
43-50: Review of Optional Dependencies ConfigurationThe new
[project.optional-dependencies]section with theregionsgroups is correctly defined. The dependency constraints (email-validator>=2.2.0andtext-region-parser>=0.1.1) align with the feature requirements, ensuring that the optional dependencies are available when needed (as indicated by the updated pre-commit hook configuration). Please confirm documentation updates to reflect these new groups.
207-214: Review of Injected Markdown Generator ConfigurationThe new markdown generator configuration for "Injected Configuration" is well-formed. The added properties—
table_only = true,file_prefix, andregion = "config"—appear to implement the intended functionality for generating a basic table output in the designated documentation file. Please verify in the generator implementation that theregionproperty is correctly handled as part of the conditional region-specific processing.
- Correct grammar issues in CLI option descriptions for `.env` file loading.
- Update documentation tables in `InjectedConfiguration.md`, `Configuration.md`, and `SimpleConfiguration.md` with corrected phrasing for `.env` descriptions.
- Clarify content generation logic in `markdown.py` for table and region options.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pydantic_settings_export/generators/markdown.py (1)
229-268: Consider optimizing region parsing implementation.While the implementation is functionally correct, consider these improvements:
- Make the error message more specific about file requirements
- Initialize the region parser once for all files to improve performance
def run(self, *settings_info: SettingsInfoModel) -> list[Path]: if not region: return super().run(*settings_info) import text_region_parser result = f"\n{self.generate(*settings_info).strip()}\n" file_paths = self.file_paths() constructor = text_region_parser.RegionConstructor() + # Initialize region parser once for all files + constructor.add_parser(cast(str, region))(lambda _: result) - # Add the region with name from the config - # This region will be replaced with the generated content - constructor.add_parser(cast(str, region))(lambda _: result) - updated_files: list[Path] = [] for path in file_paths: if not path.is_file(): raise FileNotFoundError( - f"The file {path} does not exist. " - f"Please, create this file before running the generator with the `region` option." + f"The file {path} must exist and contain region markers " + f"<!-- region:{region} --> and <!-- endregion:{region} --> " + f"before running the generator with the `region` option." )docs/Configuration.md (1)
9-9: Fix grammar in environment variable descriptions.There are a few grammar issues in the documentation:
- Missing comma after "variables" in the sentence
- "then" should be "when"
- "require" should be "requires" to agree with "class/instance"
-The path to the `.env` file to load environment variables. Useful then you have a Settings class/instance, which require values while running. +The path to the `.env` file to load environment variables, useful when you have a Settings class/instance, which requires values while running.Also applies to: 16-16, 17-17
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/Configuration.md(1 hunks)docs/InjectedConfiguration.md(1 hunks)docs/SimpleConfiguration.md(1 hunks)pydantic_settings_export/cli.py(1 hunks)pydantic_settings_export/generators/markdown.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- pydantic_settings_export/cli.py
🧰 Additional context used
🪛 LanguageTool
docs/Configuration.md
[uncategorized] ~17-~17: Possible missing comma found.
Context: ...nv` file to load environment variables. Useful then you have a Settings class/instance...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/InjectedConfiguration.md
[uncategorized] ~16-~16: Possible missing comma found.
Context: ...nv` file to load environment variables. Useful then you have a Settings class/instance...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~16-~16: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...u have a Settings class/instance, which require values while running. | null ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
docs/SimpleConfiguration.md
[uncategorized] ~9-~9: Possible missing comma found.
Context: ...nv` file to load environment variables. Useful then you have a Settings class/instance...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~9-~9: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...u have a Settings class/instance, which require values while running. | null ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🔇 Additional comments (4)
pydantic_settings_export/generators/markdown.py (2)
88-98: LGTM! Well-documented region field implementation.The new
regionfield is well-implemented with proper typing, clear documentation, and helpful examples.
116-124: LGTM! Robust optional dependency validation.The validator correctly checks for the required dependency using
importlib.util, providing a clear error message when the dependency is missing.docs/InjectedConfiguration.md (1)
1-7: Clear Introduction for Injected ConfigurationThe introductory section clearly explains the concept of injected configuration along with the region markers. This helps set the context for users on how the generated configuration will replace the marked sections.
docs/SimpleConfiguration.md (1)
1-8: Effective Presentation of Configuration TableThe table layout in this documentation is well-structured and provides a clear, concise overview of each configuration setting. The headers and formatting align well with the project’s documentation style.
- Corrected "then" to "when" in the description.
- Fixed grammar: changed "which require values" to "which requires values."
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Closes #6
What have been done:
Summary by CodeRabbit
env_filefield in CLI settings.