-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Tweak sources to have case_sensitive
and env_prefix
usable as args
#76
Conversation
self.env_prefix_len: int = env_prefix_len | ||
) -> None: | ||
super().__init__(settings_cls, case_sensitive, env_prefix) | ||
self.env_nested_delimiter = env_nested_delimiter |
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.
env_nested_delimiter
is str | None
here, isn't it an issue regarding this part?
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.
If env_name='foo'
then f'{env_name}{self.env_nested_delimiter}'
will be converted to 'fooNone'
which is not raising an error but it's better to handle it better.
we can return at the beginning of the function by checking the env_nested_delimiter is None
.
If you want to fix it, please do it in a separate PR. otherwise I will fix it
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 will take a look
Please review |
@Viicos Thanks for this PR. Please continue with the tests and documentation |
please update |
fa4d614
to
12368d1
Compare
12368d1
to
6bccb70
Compare
Basic tests added, updated docstrings and documentation (not sure the added section is required, as each section describing |
Please review |
2009274
to
36a4e5b
Compare
please update |
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
+ Coverage 97.32% 97.41% +0.08%
==========================================
Files 5 5
Lines 299 309 +10
Branches 67 67
==========================================
+ Hits 291 301 +10
Misses 6 6
Partials 2 2
☔ View full report in Codecov by Sentry. |
Please review |
Thanks @Viicos |
Fixes #74.
Ended up doing a bit of refactoring, the idea is to avoid accessing
self.config
(evenself.settings_cls.model_config
in some places, which points to the same config object) in sources, and determine the values to use in_settings_build_values
instead.Alternatively we could have some kind of "context" object, created by merging args values from
_settings_build_values
and values from themodel_config
. This way we wouldn't have to pass each config value by hand to the sources'__init__
.Waiting for review before fixing tests and updating documentation.
Selected Reviewer: @hramezani