Add recursive file and directory settings discovery.#63
Open
SerGeRybakov wants to merge 2 commits into
Open
Conversation
Allow CLI and generator configs to load BaseSettings from Python files and directories, disambiguate duplicated (same-named) settings from different sources in generated outputs.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request extends pydantic-settings-export to support importing settings from Python file paths and recursively discovering settings from directories, in addition to existing module/class path imports. It refactors import logic into a unified utility, adds source tracking to settings models, implements disambiguation for duplicate names, and updates CLI/exporter/generator flows accordingly with comprehensive test coverage. Changes
Sequence DiagramsequenceDiagram
participant Config as Config/CLI
participant ImportUtils as Import Utils
participant Discovery as Path/Module<br/>Discovery
participant Dedup as Deduplication
participant Generator as Generator
Config->>ImportUtils: import_settings_from_strings(paths)
loop For each path
ImportUtils->>Discovery: Resolve path (module/file/dir)
alt Module path
Discovery-->>ImportUtils: Import module<br/>Extract BaseSettings
else File path
Discovery->>Discovery: Load module from file<br/>via spec_from_file_location
Discovery-->>ImportUtils: Extract BaseSettings
else Directory path
Discovery->>Discovery: Recursively traverse<br/>and import .py files
Discovery-->>ImportUtils: Collect all BaseSettings
end
end
ImportUtils->>Dedup: Deduplicate by source<br/>and qualified name
Dedup-->>ImportUtils: Deduplicated settings list
ImportUtils-->>Config: Return settings list
Config->>Generator: Disambiguate duplicate<br/>names with [Source]
Generator-->>Config: Generate output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pydantic_settings_export/models.py`:
- Around line 288-307: The issue is that settings_source assumes the argument is
a BaseSettings class/instance and treats plain BaseModel instances as classes,
causing attribute access errors when getfile fails; update settings_source to
detect both BaseSettings and BaseModel instances (e.g., import and check
isinstance(settings, (BaseSettings, BaseModel))) and always set settings_class =
settings.__class__ for instances before calling getfile, so the fallback and
__qualname__ access use the class object; also mirror this change in
from_settings_model and any other callers referenced (the recursion points like
from_settings_model) so nested BaseModel instances are normalized to their class
before deriving the source.
In `@pydantic_settings_export/utils.py`:
- Around line 284-289: importlib.import_module(module_name) may return a cached
module from sys.modules that points to a different file; before returning the
imported module (inside the try in the block that uses _make_module_name and
_prepend_sys_path), validate that the module's __file__ matches the expected
path for the target settings file (use the path variable passed in) and only
return the module if it matches; if it does not match, treat it as a failed
import (fall through to the existing synthetic import fallback) or explicitly
raise so the fallback logic that creates a synthetic module executes. Ensure
this check is performed where importlib.import_module(module_name) is called and
reference the module object returned to inspect module.__file__.
In `@tests/conftest.py`:
- Around line 28-66: The fixture path properties (module_file_source,
discovered_dir_source, standalone_file_source, problematic_dir_source,
no_settings_file_source, broken_syntax_source, broken_import_source,
text_file_source) currently use f"...{Path.relative_to(... )!s}" which yields
OS-specific separators on Windows; create a small helper that calls
Path.relative_to(self.root).as_posix() and use it in each property (and for the
colliding_settings_project.sources entries) so all returned fixture source
strings are normalized to POSIX-style forward slashes for TOML embedding.
In `@tests/test_cli.py`:
- Around line 311-315: Normalize Windows backslashes before embedding paths into
the TOML test configs by calling as_posix() on Path objects used when writing
the config; specifically update the config_file.write_text calls that
interpolate output_file (and any other Path variables used in those TOML
strings) to use output_file.as_posix() so backslashes aren’t emitted and TOML
basic strings aren’t mis-parsed.
In `@tests/test_exporter.py`:
- Around line 41-51: The test test_exporter_init_default_generators currently
suppresses all warnings during Exporter() construction which hides
generator-init failures; change the warnings block to capture warnings (use
warnings.catch_warnings(record=True) and warnings.simplefilter("always")) when
calling Exporter(), then assert that the captured warnings list is empty (or
explicitly fail with the warning contents) before continuing to inspect
exporter.generators; reference Exporter() construction and
AbstractGenerator.ALL_GENERATORS so any generator initialization warnings cause
the test to fail rather than be silently ignored.
- Around line 366-393: The test
test_generator_settings_mixed_old_and_new_sources_are_deduplicated currently
only counts the exact string "AppSettings\n" which can miss duplicate sections
that are disambiguated like "AppSettings [source]"; update the assertion that
checks for duplicates (after calling Exporter.run_all via exporter.run_all()) to
count all heading lines that begin with "AppSettings" (regardless of trailing
disambiguation) — for example by matching lines that start with "AppSettings" or
using a startswith/regex on content lines — so the test fails if more than one
AppSettings section (with or without suffix) is present; keep references to
SimpleGenerator/SimpleSettings usage and the output_file content variable when
making the new assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be3e3a88-9f4e-498d-847b-fa39f77c5363
📒 Files selected for processing (19)
.pre-commit-config.yamlREADME.mdexamples/.env.exampleexamples/Configuration.mdexamples/InjectedConfiguration.mdexamples/SimpleConfiguration.mdexamples/config.example.tomlexamples/pyproject.example.tomlpydantic_settings_export/cli.pypydantic_settings_export/exporter.pypydantic_settings_export/generators/abstract.pypydantic_settings_export/generators/markdown.pypydantic_settings_export/models.pypydantic_settings_export/settings.pypydantic_settings_export/utils.pytests/conftest.pytests/test_cli.pytests/test_exporter.pytests/test_utils.py
Handle nested BaseModel instances when deriving settings sources, validate file imports against the expected module path, and normalize test/config path serialization so relative sources remain cross-platform.
Owner
|
I think it is better to do two things:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR extends settings discovery so the project can load
BaseSettingsclasses not only from Python import strings, but also from Python files and directories scanned recursively.It also fixes a long-standing ambiguity in generated outputs when multiple settings classes share the same class name but come from different files. Generated artifacts now include explicit source information for colliding top-level settings, so
.env, Markdown, simple text, and TOML outputs remain understandable and distinguishable.What changed
Added support for resolving settings sources from:
modulemodule:attribute./path/to/file.py./path/to/directory(recursive*.pyscan)Applied the new resolution logic consistently to:
default_settingssettingsextend_settingsMade relative filesystem paths resolve from
project_dirAdded deduplication so the same settings class is not processed multiple times when discovered through multiple source forms
Preserved partial-success behavior for generator-level mixed source lists:
Improved generated output for colliding top-level settings names by adding explicit source information to duplicate names
Updated docs and config field descriptions to document file and directory based discovery
Collision handling
Before this change, if two different files defined the same top-level settings class name, generated outputs could contain visually ambiguous duplicate sections such as two
AppSettingsblocks with no indication of origin.Now, when duplicate top-level names are detected in the same generation run, those settings are rendered with an explicit source suffix, for example:
AppSettings [./config/api_settings.py:AppSettings]AppSettings [./config/worker_settings.py:AppSettings]This applies across all built-in generators so that:
.envgroup headers are distinguishableUnique settings names are left unchanged.
Tests
The test suite was expanded with real filesystem-based scenarios created under
tmp_path, without mocking the core import/discovery logic.Coverage includes:
settings/extend_settingsbehaviorSummary by CodeRabbit
Release Notes
New Features
Documentation
Improvements
Chores