dvc: refactor config #3298
dvc: refactor config #3298
Conversation
|
@Suor Looks amazing! Please check the tests though, looks like slight hickup with schemas for some remotes. |
|
@efiop I see two issues:
I will fix these tomorrow. |
efiop
added
refactoring
|
@Suor Please rebase. |
|
@efiop rebased |
|
|
||
| self.extra_args = {} | ||
|
|
||
| self.sse = config.get(Config.SECTION_AWS_SSE, "") | ||
| self.sse = config.get("sse") |
There was a problem hiding this comment.
Shouldn't here be config.get("sse","")? And if not, why do we do config.get("acl","")?
There was a problem hiding this comment.
Doesn't matter really until it's boolean false it won't be used anyway.
There was a problem hiding this comment.
Removed "" for consistency.
| cache_dir = config.get(Config.SECTION_REMOTE_URL) | ||
|
|
||
| if cache_dir is not None and not os.path.isabs(cache_dir): | ||
| cwd = config[Config.PRIVATE_CWD] |
There was a problem hiding this comment.
@Suor You might've lost _cwd logic, which was resolving relative paths based on config location. Or am I missing something here?
There was a problem hiding this comment.
I thought that we have test for this 🤔
There was a problem hiding this comment.
This is not needed anymore:
relative paths are handled transparently upon load/save
There was a problem hiding this comment.
@Suor Could you please refer me to some part of the code?
Advantages:
- read like a dict
- modify as a dict for testing or manipulation purposes
- wrap modifications into context manager to save them:
with config.edit("local") as conf:
conf["core"]["remote"] = "new-default"
Design decisions:
- dropped string constants
- remote sections are parsed into a simple subdict upon load
- remote config schema is now defined by url scheme
- relative paths are handled transparently upon load/save
- user config and remote manipulations are moved to commands
- same dev manipulations are done by simply modifying a config dict
|
@Suor Please check the DS and CC reports, there are some good points there. |
dvc/config.py
Outdated
| def resolve(path): | ||
| if os.path.isabs(path) or re.match(r"\w+://", path): | ||
| return path | ||
| return RelPath(os.path.join(cwd, path)) |
There was a problem hiding this comment.
cwd is defined after this helper in which it is used. This is error-prone.
There was a problem hiding this comment.
This is minor. Nothing functionally wrong about it because of the same namespace, but causes a mental hickup.
There was a problem hiding this comment.
Defining subfunction in the middle causes mental hiccup too)
There was a problem hiding this comment.
I can move it if that worries you.
|
@efiop I actually looked them through a couple of days ago) Most of them not introduced by this PR, from the rest:
While some stylistic changes can be done, that will bring no value. Diving into DS was a waste of time again. |
👍
👍
These are worth fixing, it affects the code style, we avoid such things in other places.
👍
Style is not a waste of time for DVC, please fix those few issues left. |
|
Thanks @Suor ! 🙏 |
Advantages:
Design decisions: