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

Add bonzai.Persistence and Persister with CmdCompletion #275

Open
rwxrob opened this issue Nov 21, 2024 · 10 comments · May be fixed by #276
Open

Add bonzai.Persistence and Persister with CmdCompletion #275

rwxrob opened this issue Nov 21, 2024 · 10 comments · May be fixed by #276

Comments

@rwxrob
Copy link
Owner

rwxrob commented Nov 21, 2024

By moving the vars.Driver interface definition and adding Complete method into bonzai itself the Set/Get methods can do the work of var without requiring the var command to be imported.

@rwxrob rwxrob linked a pull request Nov 21, 2024 that will close this issue
6 tasks
@rwxrob rwxrob changed the title Move vars.Driver to bonzai, add Complete Add bonzai.Persistence and Persister with CmdCompletion Nov 21, 2024
@rwxrob
Copy link
Owner Author

rwxrob commented Nov 21, 2024

See PR #276 for ongoing development of this addition.

@Chaitanyabsprip
Copy link
Contributor

I saw the PR, even though you'll have the interface and package level variable for the Persister, you will have to import vars in anyway for the default driver, no?

To set the Persister in the user bonzai Cmd files, we'd have to add init methods, which is not as sleek as using the vars.Cmd.

We can't have the default implementation in bonzai because that pulls in a bunch of dependencies and also you can't use the default implementation (from bonzai) in vars because that will cause a circular dependency.

@rwxrob
Copy link
Owner Author

rwxrob commented Nov 21, 2024

The problem is Var.Persist. People expect it to automatically persist when set and Cmd.Set is called. The alternative is to call both Set and also something that implements persistence that is not necessarily consistent for everything in the binary. This seems like way more code than just an init() assigning to bonzai.Persistence or better, something like this:

func main() {
  bonzai.Persistence = vars.Driver
  foo.Cmd.Run()
}

Or maybe a bit of syntactical sugar:

func main() {
  foo.Cmd.WithPersistence(vars.Driver).Run()
}

The var command will still be needed, but it will probably be a vars.Editor interface that embeds bonzai.Persistence interface adding more methods for dealing with bulk editing.

Thoughts?

@rwxrob
Copy link
Owner Author

rwxrob commented Nov 21, 2024

Another thing is that comp.Vars can be implemented to use to complete var keys with different scopes, the current Cmd, or perhaps all the Cmds above it as well. We can keep vars.Comp to use completion against the entire set of persistent keys for the given binary, which may or may not all be declared with Vars.

There's another implicit design question here. Should we mandate declaration of any and all variables used in Vars now? This seems reasonable now that we have a comprehensive way of handling them. It would mean that bonzai command creators follow the same convention for such things that can be relied upon. A command that uses a variable from some other external source then becomes less than best practice unless there is data that must live outside of the Vars space. Opening external application configuration files and such is probably one solid example of this.

I do wish we could tackle the abstraction of conventional configuration with standardization on the yq/jq dialect so that I can port certain applications into a universal collection of configuration files. But that is another project. I had that before. Cobra has Viper to attempt this, but it has not been adopted at all, probably because it is lacking the yq/jq dialect. We could specify a bonzai.Configuration implementation like bonzai.Persistence but rather than being key-value pairs it would have a specific configuration name. (We can talk about all this later.)

@rwxrob
Copy link
Owner Author

rwxrob commented Nov 21, 2024

Also, what about Var.Export or Var.Scope? Now that all declared Cmd.Vars are cached privately, we can block their retrieval in Get/Set to different scopes:

  • Only the current Cmd in which Vars was declared
  • Only the immediate childen of Cmd (Cmd.Cmds and Cmd.Def)
  • All descendents in the Leaf.Path

This actually addresses a concern about security that I had before. A branch can be grafted in from an unknown source and even after compilation cannot mess with another Cmds declared Vars if the Cmd author does not want.

@Chaitanyabsprip
Copy link
Contributor

I like the scope idea, that is something you cannot have with package level variables.

@rwxrob
Copy link
Owner Author

rwxrob commented Nov 21, 2024

The scope thing also allows us to automatically document Vars based on scope and inheritance stuff.

@Chaitanyabsprip
Copy link
Contributor

I get your point about having to call both the Set(Cmd and vars.Driver) methods. That becomes inevitable. Also, I already thought that Cmd.Vars will now be the "idiomatic" way of handling variables for Cmds.

@Chaitanyabsprip
Copy link
Contributor

Are we going to allow separate persistence drivers per Cmd? You example of foo.Cmd.WithPersistence(vars.Data) hints towards that. One use case I can think of is to isolate a data heavy operation Cmd (reads from Database or remote) vs a usual operation Cmd (reads from a configuration file). You wouldn't want all your configuration to be in a full blown database when a conf file will suffice. I'm not sure whether the isolation of packages will allow one persistence driver per command.

@Chaitanyabsprip
Copy link
Contributor

Allowing that would also require us to have good defaults. If it's not set in all node Cmds of a tree then should it assume the Driver of the parent Cmd?

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 a pull request may close this issue.

2 participants