Closed z3z1ma closed 2 years ago
@gouline
Ready for review. This one is a good one.
Looks cool. Was this trying to solve any particular issue or just overall user friendliness?
We should really start stabilising the 0.8.0 interface and documentation, my concern is that we're up to RC4 and we're still making breaking changes...
Yes this one was fully focused on user friendliness.
For the end user there is actually no change in this PR unless they are invoking the package through python in which case it definitely would be a breaking change. I was highly intentful on making the package work identically with the refactored CLI as it did before.
The feature addition adds a new config
command which lets users interactively generate a config which essentially caches arguments they would have to repeat on most invocations. Its totally optional for the end user though. In addition we support env vars but those are also completely optional.
AFAIK we are only adding enhancements and no breaking changes.
That makes sense. For breaking changes, I meant if they're invoking it programmatically in Python.
Once this PR is merged, let's not have any more changes for 0.8.x. I will release RC5, we can wait a few weeks to allow for finding/fixing any bugs and then I will release the final 0.8.0. Other features, such as metrics parsing, can go into 0.9.0 alpha/beta builds.
You got it. I appreciate your time and guidance.
Alright.
I reinstated the config objects, which can be used as they were, and moved the interfaces to a separate module. The interfaces now inherit directly from the configs. This object inheritance is much more standard and clean practice. This makes it extremely clear that the interfaces extend configs with the addition of class methods and props that integrate their respective parsers/client. So, as a user in Python, importing and using the interface means you don't need to build, then validate, then shuttle the config to the other classes which ingest them. The interface will give you access through props to a resolved parser, or client, perform validation, etc. The higher level things that weren't in scope of a config class alone.
The whole codebase is a treat to digest right now I think. But I defer to others to see if they share the sentiment 😄
I think there is just documentation updates and anything else that can come from your review before its buttoned up and we leave it to stew.
Love your work. Looks good overall, I think it's ready for the documentation updates and then I'll do the final pass review before merging.
@gouline
Documentation updates complete including the config section for end users too.
Description
Large refactor to use click as cli interface, improve and further abstract interface complexity, allow env vars implicitly, improve logging with global logger + rich, add config yaml with interactive setup via config command, load config via hook in CLI, split commands in sub commands, optimize help text, add implicit file / folder existence checking on file / folder args.
Sub commands broken out:
Optimized help text
Interactive config file generation:
Checking config file contents with simple
dbt-metabase config --inspect
and running a CLI command with justdbt-metabase models
with auto config loading:An example config:
Type of change
How Has This Been Tested?
Existing unit tests as well as production use. In addition, this time around using
nox
I have tested the package on Python 3.6, 3.7 and 3.8Test Configuration:
Checklist:
Closing Notes
Env Vars taking precedence over config.yml defaults (via injection) CLI flags taking highest precedence as in a standard CLI tool (overrides)