edgedb / edgedb-cli

The EdgeDB CLI
https://www.edgedb.com/docs/cli/index
Apache License 2.0
165 stars 23 forks source link

Un-hardcode configuration parameters from `edgedb-repl/commands/configure` #3

Closed elprans closed 4 years ago

elprans commented 4 years ago

Currently, the configuration properties are hardcoded. This is suboptimal because the number of config options will grow, and adding new options will require new CLI releases. Consider implementing the CONFIGURE calls the way they are in the Python REPL.

tailhook commented 4 years ago

Well, for things like Auth and Port we definitely need adequate --help. So it makes much more sense to have them hardcoded.

For smaller things maybe, but I don't want --help to require connection and it's more likely that lots of small tuning settings would be applied by cat settings.edgeql | edgedb rather than using command-line anyway.

Also hardcoding is the only adequate way to provide completion (completion requiring a database connection is surely a big "no").

I don't see releasing a new REPL with a new version of EdgeDB is a major issue. We would likely do that anyway because of other features.

elprans commented 4 years ago

OK, then we should implement a codegen similar to that used for errors to alleviate the maintenance burden and reduce the chance of bugs.

tailhook commented 4 years ago

There is a codegen for command-line options, so adding an option is adding two things, an option with description:

    /// The amount of memory used by internal query operations such as sorting.
    ///
    /// Corresponds to the PostgreSQL work_mem configuration parameter.
    QueryWorkMem(ConfigStr),

And a match case:

        C::Set(Set { parameter: S::QueryWorkMem(param) }) => {
            set_string(cli, "query_work_mem", param).await
        }

Which is kinda simple enough. Unfortunately resetting option takes another two things:

    /// Reset work_mem postgres configuration parameter to default value
    QueryWorkMem,
                C::QueryWorkMem => "query_work_mem",

So generally:

  1. For simple options, code boils down to name, type and description
  2. Complex options like listen_addresses require their own code anyway

So I think more codegen would only complicate the code and maintenance.

On the other hand, we can make a test which runs configure set --help and checks that all options are present.

elprans commented 4 years ago

@tailhook I still don't think that autocomplete and help are worth the extra effort of maintaining explicit enums in CLI in the long term, but I'm fine with keeping the status quo until (and if) this becomes too annoying. Adding a test for CLI exhaustiveness would be a good way of keeping it in shape for now.