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

Decentralized module loading and argument definitions #121

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dbennett-sentry
Copy link
Contributor

@dbennett-sentry dbennett-sentry commented Apr 22, 2024

The biggest change here is dynamically loading all commands from a target repository, a user dir, or even the devenv package itself. All commands behave similarly at this point and we can now abstractly manage and report on errors or even interactions.

A simplified argument model has been provided to encourage the creation of new commands without having to wade through the details of argparse and, more importantly for UX, it provides full CLI help up front vs applying local argparse configurations later via processing of arguments after a module has been selected. The remaining arguments are still forwarded for constrained or exceptional use-cases and the developer can still write arg-parse directly via the remaining arguments -- at the cost of help support.

@dbennett-sentry dbennett-sentry force-pushed the dbennett/feat-dynamically-loaded-modules-and-args branch 4 times, most recently from 38353ed to 1c20441 Compare April 23, 2024 21:28
command="doctor",
help="Diagnose common issues, and optionally try to fix them.",
)
module_info = ModuleDef(module_name=__name__, name="doctor", help="doctor")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to update the help on these commands.

@dbennett-sentry dbennett-sentry force-pushed the dbennett/feat-dynamically-loaded-modules-and-args branch 2 times, most recently from fa1b290 to 9e37e8d Compare April 24, 2024 18:56
@asottile-sentry
Copy link
Member

2c: this seems "cute" but going to be a pain to debug. I think we should just be using argparse subparsers directly -- it'll be simpler, easier to understand, and much much easier to debug and reason about -- can make simple mutators like this for any common setup

or if we want a more involved cli framework (I don't think we do?) we should just use something that exists like click (I don't really like personally) rather than writing our own. I don't think our subpcommands are complex enough to warrant one though

@dbennett-sentry dbennett-sentry force-pushed the dbennett/feat-dynamically-loaded-modules-and-args branch from 9e37e8d to 7f9e02a Compare April 24, 2024 19:22
@dbennett-sentry dbennett-sentry force-pushed the dbennett/feat-dynamically-loaded-modules-and-args branch from 7f9e02a to 74e509d Compare April 24, 2024 19:23
@dbennett-sentry dbennett-sentry marked this pull request as ready for review April 24, 2024 19:26
@dbennett-sentry
Copy link
Contributor Author

dbennett-sentry commented Apr 24, 2024

2c: this seems "cute" but going to be a pain to debug. I think we should just be using argparse subparsers directly -- it'll be simpler, easier to understand, and much much easier to debug and reason about -- can make simple mutators like this for any common setup

The goal isn't to be cute, it's to reduce friction to every other dev to contribute but to do it in an opinionated way. Argparse isn't the friendliest, and argparse subparsers add new weirdness. I wanted a pidgin argument capability, not an all-encompassing alternative.

Let me take a look at your counter-offer, though, as it's stood a bit more of a test of time than what I have tossed together here.

@IanWoodard
Copy link
Member

2c: this seems "cute" but going to be a pain to debug. I think we should just be using argparse subparsers directly -- it'll be simpler, easier to understand, and much much easier to debug and reason about -- can make simple mutators like this for any common setup

The goal isn't to be cute, it's to reduce friction to every other dev to contribute but to do it in an opinionated way. Argparse isn't the friendliest, and argparse subparsers add new weirdness. I wanted a pidgin argument capability, not an all-encompassing alternative.

Let me take a look at your counter-offer, though, as it's stood a bit more of a test of time than what I have tossed together here.

I appreciate how we’ve managed to keep main.py clean and streamlined the process of adding new arguments without needing to modify multiple files. However, I do see where Anthony is coming from regarding the complexity introduced by our custom parsing layer.

When looking at our _convert_argument_params function and the way we wrap argparse functionalities, I wonder if we might streamline these processes a bit. Simplifying or even reducing some of the custom parsing steps, while maintaining our decorator-based interface, could help keep the system both easy for devs to contribute to and more maintainable overall.

What do you think about refining the interaction between our decorators and argparse subparsers to simplify things? I believe this could help us preserve the developer-friendly nature of our code, while also making it easier to maintain and debug.

@dbennett-sentry
Copy link
Contributor Author

dbennett-sentry commented Apr 25, 2024

What do you think about refining the interaction between our decorators and argparse subparsers to simplify things? I believe this could help us preserve the developer-friendly nature of our code, while also making it easier to maintain and debug.

Feel free to give it a go. Requirements are:

  1. extremely low friction for external developers wishing to create commands;
  2. opinionated and consistent outcomes by default;
  3. escape hatches for power-users.

@asottile-sentry
Copy link
Member

personally I don't think we want low friction for outsiders adding commands to devenv -- we want to keep them generic and opinionated

@dbennett-sentry
Copy link
Contributor Author

personally I don't think we want low friction for outsiders adding commands to devenv -- we want to keep them generic and opinionated

That's a difference we'll have to chat through. We'll never get alignment on code if we have differing goals.

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.

3 participants