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
Refactors config files #720
Conversation
can you review this @idomic ? |
src/ploomber/config.py
Outdated
|
||
|
||
class Config(abc.ABC): | ||
"""And abstract class to create configuration files (stored as YAML) |
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.
Typo
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.
fixed
return Path(check_dir_exist(CONF_DIR), DEFAULT_USER_CONF) | ||
|
||
|
||
class Internal(Config): |
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.
Maybe call it ploomber settings?
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 want to be more explicit that these are for internal use and others are for users.
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.
Looks good besides comments
assert telemetry.check_first_time_usage() | ||
assert not telemetry.check_first_time_usage() | ||
|
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.
How does this pass, now that we don't set the conf?
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 added a one-off flag to the config to replicate this functionality
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.
Cool just make sure it works via the dashboard
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.
Just make sure to test/squash commits.
Besides, lgtm
* deletes empty function * reorders imports * functions to return paths to config files * simplifying config file read * adding Internal and Storage classes * simplifying writes * renaming * adds Config * improvements to Config * refactors telemetry and cloud to use new Config * documents config * improves warning messages * flake8 * validating config types * fixes typo * flake8
I refactored the logic to store and retrieve the data from the config files. This is a pre-requisite to start working on the improved onboarding workflow where we have a "tutorial" to point to the user what do to next since we need to add a few extra flags to the config files to track the state.
The tests remain unchanged (I only modified a few and deleted some that are outdated)