Skip to content
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

Refactor/465 cmd config rework #495

Merged
merged 14 commits into from
Oct 15, 2023
Merged

Conversation

pharr117
Copy link
Collaborator

Scope of this PR

This PR completely reworks the way commands receive the configuration values associated with them. It does this by splitting each command configuration out into its own block, allowing for per-command configuration requirements.

Old Methods

The old method used a single configuration setup for the entire application. This means every command required the exact same configuration, with each command only using the configs it needed to.

The commands also used the same setup function, which was an end-to-end setup of database connections, loggers etc...

New Methods

This PR introduces the following new behavior:

  1. Split of all configuration setups into discrete blocks
  2. Generalization of common configs and common validation methods for code reuse
  3. New PreRun functions for every command that handles getting and validating command configurations
  4. Removal of app-wide dependency on single setup function, each command only sets up what it needs

This achieves the following:

  1. Better code reuse
  2. Discrete commands allow for easier debugging and less downstream affects if things change
  3. Easier locations to establish common configurations and validations
  4. Better --help output for each command
  5. Smaller/better scoped config requirements for each command

@pharr117 pharr117 added the enhancement New feature or request label Oct 11, 2023
@pharr117 pharr117 requested a review from danbryan October 11, 2023 03:35
@danbryan
Copy link
Collaborator

Hey, lets run over this in person before i merge it in as you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants