-
Notifications
You must be signed in to change notification settings - Fork 2
enh: Support duct config files #305
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
Conversation
use default log level until we parse the config and CLI
🧪 CI InsightsHere's what we observed from your CI run for 0fb8b2c. 🟢 All jobs passed!But CI Insights is watching 👀 |
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.
Pull Request Overview
This PR adds comprehensive configuration file support to the con-duct tool, allowing users to configure options through JSON files, environment variables, or command line arguments with proper precedence handling.
- Implements a configuration system with precedence: defaults < config files < environment variables < CLI arguments
- Refactors argument parsing to use a custom
ConfigurableArgumentParser
that integrates with the new config system - Updates the help text to reflect configuration capabilities and show current default values
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/con_duct/main.py | Core implementation of config system including Config class, ConfigurableArgumentParser , and integration with argument parsing |
src/con_duct/suite/main.py | Added TODO comments for future config integration in the suite subcommands |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@yarikoptic also mergify says green but tests failed... now that I think about it, I'm not sure if I've seen mergify comments that were helpful, have you? |
configuration: | ||
Many options can be configured via JSON config file (--config), environment | ||
variables, or command line arguments. Precedence: built-in defaults < config | ||
file < environment variables < command line arguments. |
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.
file < environment variables < command line arguments. | |
file(s) < environment variables < command line arguments. |
per directory, per user, system?
I think we decided to go with jsonargparse and thus this PR could be closed |
@yarikoptic this is a WIP, but non-draft since this could use design review before continuing into the weeds
TODO (adjust) At the high level:
DEFAULT_CONFIG
defined in__main__
-C --config
first if passedNOTE! If you use config files, duct --help will show options' default values to be the ones specified in the config, which I think makes sense but a little unusual.
TODO:
config
everywhere instead ofargs
. IMO nicer usage, but uglier code and a substantially large patch so not worth itcon-duct init-conf
to scaffold a config.json file with all default values.