Refactor the configuration parsing #513
Conversation
Deploying datachain-documentation with
|
| Latest commit: |
0150437
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3d2a9b2b.datachain-documentation.pages.dev |
| Branch Preview URL: | https://amrit-config.datachain-documentation.pages.dev |
As a part of #10774, this introduces a process to save the configuration in local, system and global configuration. The precedence of the level are as: - system - global - local Local configuration overrides global and so on. This borrows the logic of how configuration is managed in DVC.
amritghimire
force-pushed
the
amrit/config
branch
from
401a1df to
0150437
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #513 +/- ##
==========================================
- Coverage 87.25% 87.16% -0.09%
==========================================
Files 96 96
Lines 9931 9991 +60
Branches 1359 1367 +8
==========================================
+ Hits 8665 8709 +44
- Misses 915 928 +13
- Partials 351 354 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| ) | ||
|
|
||
| if remote_type == "http": | ||
| for key in ["url", "username", "token"]: |
There was a problem hiding this comment.
Just wonder, should we also have team_id as a part of remote config? User can be a member of many teams in Studio 🤔 Or team_id will be required as a (required? optional?) param in API calls?
There was a problem hiding this comment.
This may be part of future changes when implementing some functionality with the token access.
| except KeyError: | ||
| raise Exception( | ||
| f"config section remote.{remote} of type {remote_type} " | ||
| f"must contain key {key}" | ||
| ) from None |
There was a problem hiding this comment.
Just a comment: Studio URL may be optional 👀
There was a problem hiding this comment.
I was not sure if we will continue using this TBH. Keeping this part as it is for now.
| return load(f) | ||
| except FileNotFoundError: | ||
| return None | ||
| class Config: |
There was a problem hiding this comment.
Looks good to me, it would be great to add tests for this class.
There was a problem hiding this comment.
Added tests and fixes in #514
| # In the order of precedence | ||
| LEVELS = SYSTEM_LEVELS + LOCAL_LEVELS | ||
|
|
||
| CONFIG = "config" |
There was a problem hiding this comment.
Not sure if I understand the reason behind this attribute 👀 Can't find any usage of this.
|
|
||
| def __init__( | ||
| self, | ||
| level: Optional[str] = None, |
There was a problem hiding this comment.
Just wonder, will it be better from typing prospective to have level as enum, let's say? 👀
As a part of #10774 , this introduces a process to save the configuration
in local, system and global configuration.
The precedence of the level are as:
Local configuration overrides global and so on.
This borrows the logic of how configuration is managed in DVC.