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

feat: Add trait for extending the cli with custom inspectors #513

Closed
wants to merge 2 commits into from
Closed

feat: Add trait for extending the cli with custom inspectors #513

wants to merge 2 commits into from

Conversation

emostov
Copy link

@emostov emostov commented Mar 17, 2024

closes #503

This PR adds the trait InspectorCliExt, which allows a user to define a function that initializes and returns their custom inspectors.

Future proofing the public api - In order to keep the diff small and not over engineer for a problem we don't currently have, I avoided making a super trait for all cli extensions. However I think a super trait could help reduce the churn for brontes lib consumers and it might be wise to add sooner then later. For example we could have a trait

trait CliExt {
   type Inspector: InspectorCliExt;
   
   // Future assoiated types with default impls 
   // could be added here, minimizing the changeset for 
   // lib consumers to upgrade e.g. they avoid having to thread
   // additional generic type params across the cli code when
   // we add an additional ext trait
}
impl CliExt for () {
  type Inspector = NoopInspectorCliExt;
}

Proposed follow ups:

  • Add example - I already have a wip here https://github.com/emostov/brontes/pull/1 (lmk if you can't see it, I just needed to do it in my fork so I could target this branch)
    • An example could be good for adding some cheap sanity check test such as checking for initialized custom inspectors logging to stdout
  • Add a .env.sample to help people boot strap a local setup. Relates to Write documentation  #205,  specifically "Installation and setup instructions"
  • Expose public Cli struct that can wraps up the logic of calling Arg::parse and run, the ext trait could be an arg directly to the struct.
  • Ensure that panic messages make their way to tracing subscriber or at least stdout/stderr. It appears panic messages get squashed, which is especially burdensome because a lot of initialization logic uses unwraps/expects and it can be hard to debug. For example, the default impl for ClickHouseConfig (which is implicitly called by Clickhouse::default) will panic if certain env vars are not set, but the expect messages don't make their way to the terminal. This

Didn't want to spam with issues, but happy to break these out if they look reasonable

TODO

  • tests
  • rust docs example for trait impl

@@ -32,8 +35,8 @@ fn main() -> eyre::Result<()> {
}
}

fn run() -> eyre::Result<()> {
let opt = Args::parse();
fn run<Ext: InspectorCliExt + clap::Args>() -> eyre::Result<()> {
Copy link
Author

@emostov emostov Mar 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid all the extra + clap::Args by making InspectorCliExt a super trait of clap::Args - I don't feel strongly either way

@emostov emostov closed this by deleting the head repository Apr 8, 2024
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.

feat: Add CLI extensions so users can run custom inspectors & classifiers without forking.
1 participant