Conversation
|
|
||
| While these commands are quite handy to have inside REPL, their usefulness | ||
| as standalone CLI tools is questionable, especially when an arbitrary EdgeQL | ||
| introspection query (or even shortcuts like ``\la``) can be easily piped into |
There was a problem hiding this comment.
We don't allow piping backslash commands for now, they work only in interactive mode. Allowing them opens a large can of questions.
| It is convenient to expose the EdgeDB CLI commands directly in REPL. This | ||
| can greatly simplify administrative tasks when the DB administrator has full | ||
| DB admin rights but yet can't access the server shell. For that purpose, all | ||
| CLI commands are exposed with the ``!`` prefix, similar to how IPython |
There was a problem hiding this comment.
I don't agree that two prefixes with different sets of commands are a good thing here. Each thing needs to be remembered. Every time i would like to make a command I would probably try both. And like with USB's law I would only succeed a third time (perhaps because I've misspelled something).
Also amount of code to support this is twice as big, including all the hinting and completion.
|
|
||
|
|
||
| Connection Options | ||
| ------------------ |
There was a problem hiding this comment.
Can connection options can be passed via !commands?
There was a problem hiding this comment.
Second problem is these arguments:
--password Ask for password on the terminal (TTY)
--password-from-stdin Read the password from stdin rather than TTY (useful for scripts)
Because they are technically also connection arguments. And same names are reused in create role / alter role.
There was a problem hiding this comment.
Third problem is that it's unsupported in clap: clap-rs/clap#1385
(even if when it becomes supported I'm not sure about the granularity of that setting, as it may apply to all arguments rather than selected ones). The implementation of the hoisting connection arguments up would require another piece of code for each command.
Surely, we can try to contribute the feature, but given the scope of clap (i.e. all kind of completion generation and so on) I'm not sure how hard it would be to make a patch and have it upstreamed.
Of course, these technical issues are fixable eventually, just noting that it isn't a very simple task.
There was a problem hiding this comment.
Another issue is if we go with dump all then dump all --help will list -d database which is quite weird.
|
|
||
| The RFC proposes to adopt the ``<group> <command>`` naming scheme for | ||
| the ``edgedb`` CLI. Here's an outline of current and future ``edgedb`` | ||
| subcommands: |
There was a problem hiding this comment.
Is this is an example or an exhaustive list?
What to do with list-ports? Is it ports list? Or is it config ports list or is it config list-ports or is it config show Port?
I also still believe that list-types and list-indexes and similar things can be useful in scripts. Forcing people to use echo \list-types | edgedb .. is every inconvenient. Note: you can't use two commands like that in a "script" anyway, because parsing the output would be a nightmare.
|
While initially I was proponent of subcommand appoach, now I find current approach more consistent:
So my approach to consistency is:
|
|
That said, we discussed that we want migrations be accessible in repl, so Or even |
| The last observation is what prompted the creation of this RFC. We either | ||
| need to update RFC 1001 to use the ``<action>-<category>`` scheme or | ||
| adopt the ``<group> <command>`` for all other commands. Combining two | ||
| drastically different CLI designs in one tool is not an option. |
There was a problem hiding this comment.
It depends on how you look at it. edgedb server was largely conceived as a separate tool, I think of edgedb server install not as <group> <command> but as edgedb-server <action>-<category>. I can also see how it may be interpreted otherwise, though.
| * ``config [--system]`` (used to be ``edgedb configure``) | ||
| - ``set`` | ||
| - ``reset`` | ||
| - ``add`` (used to be ``insert``) |
There was a problem hiding this comment.
Can you elaborate why you think breaking consistency with the server command here is an improvement?
There was a problem hiding this comment.
Hm, in what way is the consistency broken here?
There was a problem hiding this comment.
We don't have the CONFIGURE SYSTEM ADD syntax, but we do have CONFIGURE SYSTEM INSERT, and changing the action to add looks arbitrary to me. I know this proposal eschews the notion of resembling EdgeQL, but I don't think that using different keywords for the same operation is a good idea still.
|
+1 to the RFC in general. The AWS CLI follows the |
|
There's no mention of the redesign discussion anywhere but to confirm, the documentation lists the following env variable:
The env variable source code dates to September 2016 so it's been quite a while. |
OT: This is actually news to me. I always used and continue to use |
I second this. I haven't ever seen a guide that shows This RFC mentions only |
|
As one data point there is a |
|
Was it merged by accident? There are tons of unanswered questions here. |
|
Not by accident. After discussing this over Slack we decided to keep the current design and not make sweeping changes. I wanted to keep Yury as the original author and keep the history of the document in Git so I merged this and will post a follow-up PR shortly with the decided state of things. |
4 participants
No description provided.