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

Config command defaults (e.g. from toml or yaml file) #165

Merged
merged 30 commits into from
Jun 10, 2024

Conversation

BrianPugh
Copy link
Owner

This PR aims to make it easier to supply configuration defaults from a configuration file by introducing a new cyclopts.App parameter called bound_args_transform.

bound_args_transform is a callable (or list of callables) that has signature:

 def bound_args_transform(commands: Tuple[str, ...], bound: inspect.BoundArguments) -> Any:
     ...

The supplied functions should add/modify the provided bound object based on some external configuration. The commands parameter are the string CLI commands/subcommands that lead up to the function-to-be-executed. For example,

# assuming "cmd" and "subcmd" are cyclopts commands.
$python my-script.py cmd subcmd --foo=bar

would result in commands == ("cmd", "subcmd")

Cyclopts has a few builtin transform classes for common situations for yaml, toml, and json. Example usage:

from cyclopts import App, bound_args_transforms

app = App(
    bound_args_transform=bound_args_transforms.Toml("pyproject.toml", root_keys=["tool", "mytoolname"])
)

Extending the above example, this would:

  1. Search for pyproject.toml in the current directory. If it's not found, do not modify the bound object at all.
  2. If it does exist, look inside for the table [tool.mytoolname.cmd.subcmd] and supply the key/value pairs for unset parameters.

Other nice parameters of the builtin transforms:

  • must_exist=False: whether or not the configuration file must exist.
  • search_parents=False: iteratively search parenting directories until a config file is found.

Resolves #87. @BrendanJM what do you think about the concepts outlined in this post? This branch is not in a functional state yet (and the implementation is not yet ready to be reviewed), but I was wondering if I could get your high level feedback?

@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2024

Codecov Report

Attention: Patch coverage is 94.02390% with 15 lines in your changes missing coverage. Please review.

Project coverage is 95.87%. Comparing base (1f5e3d6) to head (bca4db5).
Report is 1 commits behind head on main.

Files Patch % Lines
cyclopts/bind.py 93.93% 2 Missing and 2 partials ⚠️
cyclopts/config/_common.py 93.54% 3 Missing and 1 partial ⚠️
cyclopts/config/_env.py 82.60% 3 Missing and 1 partial ⚠️
cyclopts/utils.py 80.00% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
- Coverage   96.40%   95.87%   -0.54%     
==========================================
  Files          16       23       +7     
  Lines        1864     2060     +196     
  Branches      468      523      +55     
==========================================
+ Hits         1797     1975     +178     
- Misses         29       43      +14     
- Partials       38       42       +4     
Flag Coverage Δ
unittests 95.67% <94.02%> (-0.52%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BrendanJM
Copy link

Hi Brian - this looks great! Looking through this, a few thoughts:

  • I like that various config formats could be used. Though focusing on only e.g. pyproject.toml seems like it could also be valid and idiomatic.
  • When searching for e.g. pyproject.toml in the current directory, is that relative to where the command is run? I imagine looking for this in the repo root by default would potentially be desirable (and maybe obviate the need to search parents). Is there conventional process for locating this (e.g. what setuptools uses, etc)?
  • [nit] The interface looks good, but to a new/basic user, bound_args_transform may benefit from a more descriptive/simplified name. But this also isn't a big deal given decent documentation and examples.

@BrianPugh
Copy link
Owner Author

Thanks for looking!

Though focusing on only e.g. pyproject.toml seems like it could also be valid and idiomatic.

I kinda agree, however there are many ways to interpret pyproject.toml that may be project-specific. The default implementation included in Cyclopts is probably generically useful, but people may want to tweak it for their project. The natural conclusion is this function-callable interface where the user can parse things however they would like.

When searching for e.g. pyproject.toml in the current directory, is that relative to where the command is run?

Yes. But again, this is project specific. For example, if we write a CLI tool that lints a repo based on it's pyproject.toml, we would want:

app = App(
    bound_args_transform=bound_args_transforms.Toml("pyproject.toml", root_keys=["tool", "mytoolname"], search_parents=True)
)

However, some other tools may want to look in some home config directory:

app = App(
    bound_args_transform=bound_args_transforms.Toml("~/.config/mytoolname/config.toml")
)

In either case, it's pretty explicit about where the configuration will be coming from.

bound_args_transform may benefit from a more descriptive/simplified name.

I 100% agree, but I couldn't think of a better name. Totally open to suggestions! A odd nuance/coincidence is that this hook is VERY similar to App.converter, just with a slightly different interface. It might be worth combining them in a non-backwards compatible way.

@BrianPugh
Copy link
Owner Author

After a little more time, i have the following thoughts:

  • This should NOT be merged with App.converter, since App.converter is NOT inherited from the parenting app. This new feature should be inherited all the way through.
  • I think we should give this the much simpler parameter name config, since that's the primary use-case. It'll also be much easier for people to find the feature.

@BrendanJM
Copy link

Agree - I think the simple config is a great name here, and would be a lot more clear in terms of intent.

@BrianPugh BrianPugh changed the title Bound args transform Config command defaults (e.g. from toml or yaml file) May 7, 2024
@BrianPugh BrianPugh marked this pull request as ready for review June 8, 2024 15:34
@BrianPugh BrianPugh merged commit b1e7883 into main Jun 10, 2024
15 checks passed
@BrianPugh BrianPugh deleted the bound-args-transform branch June 10, 2024 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discussion] First-class config overrides via pyproject.toml?
3 participants