-
Notifications
You must be signed in to change notification settings - Fork 40
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
Consolidate config parsing and validation #568
Consolidate config parsing and validation #568
Conversation
Codecov Report
@@ Coverage Diff @@
## master #568 +/- ##
==========================================
+ Coverage 86.81% 87.54% +0.72%
==========================================
Files 24 26 +2
Lines 3550 3677 +127
==========================================
+ Hits 3082 3219 +137
+ Misses 468 458 -10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
89eab98
to
88b8774
Compare
Add `RequestRetryConfig.to_urllib3_retry`.
In main code and in tests.
88b8774
to
c468773
Compare
7d529b5
to
259c616
Compare
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.
LGTM, as far as I can tell after a quick review. Just a few comments.
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.
LGTM.
Noting that you promised "We can update docs in a follow-up PR". For posterity.
@classmethod | ||
def from_config_file(cls, config_file: Optional[str] = None, | ||
profile: Optional[str] = None, **kwargs): | ||
"""Create class instance based on config file / environment / kwarg options |
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 found this a bit confusing: the class loads up on info from the file and then runs the usual from_client_config
to get what that always gets, but from this line I expected this class to deal with env vars. Can you slightly update for clairty?
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.
load_config()
few lines below actually loads from file and env. What's confusing is this method's name, but this is the best name I could come up with. I don't expect this method to get much usage.. The one you'll want to use is from_config
-- which dispatches config loading to this method. And then the name (from_config
) and the behavior is consistent with Client.from_config
.
Co-authored-by: Theodor Isacsson <[email protected]>
2fe686c
to
60b3a9e
Compare
Previously, when `config_file` was specified as a keyword argument, it would be duplicated by the old code.
This PR primarily addresses #504 (decoupling config parsing/validation from
Client
), and then addsfrom_config
factories to low-level API clients and resources (#572).To simplify config parsing, validation and usage going forward we also define a new
dataclass
-like model for the configuration,ClientConfig
(Pydantic-based).We also prepare ground for config file v2, a hierarchical config format (#426).
Close #504.
Close #572.
Close #505.
Fix #507.