docker.el

docker.el copied to clipboard

Hi there,

I have an idea for how using something like the image --mount option might feel smoother. The issue I have is that when you have to enter multiple mounts you have to select the mount option and then enter the mount switch as part of the value for the extra mounts. This feels a little clunky to me.

My suggestion is to have it so that if you click on the mount option, you enter a single value. If you click on it again, you enter the next value. If you enter an empty string, it pops the last value off of the list.

I don't mind submitting a pull request, but I thought I'd post a simple mock up here to see if you like the feel of it.

If you load this in and run M-x test-transient you can select the o option multiple times and then the r option to simply print out what you've collected.

You can see that it's done by simply supplying this new transient-infix class to the option with :class transient-multi-option. (Not to be confused with options which select from a fixed set of multiple values.)

Please let me know what you think. Regards,

(require 'dash)
(require 'transient)
(defclass transient-multi-option (transient-option) ()
  "Class used for command-line argument that can take a value.")

(cl-defmethod transient-init-value ((obj transient-multi-option))
  (progn
    (oset obj value nil)
    (oset obj always-read t)))

(cl-defmethod transient-infix-value ((obj transient-option))
  "Return (concat ARGUMENT VALUE) or nil.

ARGUMENT and VALUE are the values of the respective slots of OBJ.
If VALUE is nil, then return nil.  VALUE may be the empty string,
which is not the same as nil."
  (when-let ((value (oref obj value)))
    (--> value
           (reverse it)
           (-map (lambda (x) (concat (oref obj argument) x)) it)
           (mapconcat #'identity it " "))))

(cl-defmethod transient-format-value ((obj transient-multi-option))
  (let ((value (oref obj value)))
    (propertize (or (transient-infix-value obj) (oref obj argument))
                'face (if value
                          'transient-value
                        'transient-inactive-value))))

(cl-defmethod transient-infix-set ((obj transient-multi-option) value)
  "Set the value of infix object OBJ to value."

  (let ((orig (oref obj value)))
    (if (eq value nil)
        (if (not (eq orig nil))
            (oset obj value (cdr orig)))
    (oset obj value (cons value orig)))))
  
(defun doug-interactive ()
  (interactive)
  (message (format "%s" (transient-args current-transient-command)))
  )

(require 'transient)
(transient-define-prefix test-transient ()
  "documentation"
  ["Section header"
   ("o" "Option Description" "--option=" :class transient-multi-option)
   ("r" "Action item" doug-interactive)
   ])

One thing I noticed is that history doesn't work so great with it (yet!). Please let me know if you think this is something you'd be interested in so I can figure out if this is worth spending time on.

That looks neat! That is indeed a problem that pops from time to time (#68) and we have another issue that would be directly solved by this: #137

@maaroen would that work for you?

@Silex - Thanks for the reference. I’ll have a look at @tarsius’s suggestion.

I had a look and noticed that if you make a mistake here, then Emacs ends up in a inconsistent state, and also that this should work out of the box for transient-option but doesn't (leading to the inconsistent state). I have fixed both issues but have to wrap up things. I recommend you wait until tomorrow evening before proceeding.

@tarsius - For clarity, are you saying that transient-option class shouldn’t clear results when reselected as per the always-read slot set to nil but rather append values like the example above? Are you changing transient.el directly to have the behavior of the example above? FWIW, I think this makes much more sense as an opt-in.

I’m also not clear what kind of mistake you mean. The code above was just a rough draft.

@tarsius - For clarity, are you saying that transient-option class shouldn’t clear results when reselected as per the always-read slot set to nil but rather append values like the example above?

When using the approach that I would use, then this slot doesn't matter. So I wasn't trying to say anything about it.

Are you changing transient.el directly to have the behavior of the example above? FWIW, I think this makes much more sense as an opt-in.

Yes, here (on next): https://github.com/magit/transient/commit/2395f1f455fc4d7d7d7e0002a78b59e9ae33b891.

I’m also not clear what kind of mistake you mean. The code above was just a rough draft.

If you make a mistake writing a transient-infix-value, which results in an error being raised, then Emacs ends up in an inconsistent state and you have to kill it.

This shouldn't happen. Transient should catch that error and handle it gracefully. I have implemented that (again on next) in https://github.com/magit/transient/commit/9b0336e975e883d78e454a405921cfeeba2dd64a.

Once the first commit has been applied, then you can do:

(transient-define-argument transient-toys--animals-argument ()
  "Animal picker"
  :argument "--animal="
  :shortarg "-a"
  :description "Animals"
  :multi-value 'concat
  :class 'transient-option
  :choices '("fox" "kitten" "peregrine" "otter"))

(transient-define-prefix transient-toys-animals ()
  "Select animal"
  ["Arguments"
   (transient-toys--animals-argument)
   ("s" "show args"
    (lambda (args)
      (interactive (list (transient-args 'transient-toys-animals)))
      (message "args: %S" args)))])

(transient-toys-animals)

If, before applying the mentioned commits, you evaluate that, set the option (one or multiple values does not matter) and then invoke "show args", then an error would be triggered by the old implementation of transient-infix-value(transient-option) because it isn't up to the task yet, and then that error would mess up Emacs because there isn't any code in place yet to handle this gracefully.

You can start writing code similar to the above if you use the next branch.

@tarsius - Just to be clear, the example in the original post isn’t about selecting from a fixed set of choices at all. It’s for repeated prompting for user entered values. In this case if you don’t explicitly enable always-read you can’t possibly prompt for a second value.

To belabor the point, the set of possible values for a given option isn’t always known beforehand. See the examples @Silex refers to above. Setting environment variables is just one example.

@tarsius - Oh, I also had to inherit from transient-option instead of transient-argument to not get tripped up by transient--ensure-infix but that feels hacky. Would it be better for both transient-option and transient-switch to inherit from a common base class which can be tested in transient--ensure-infix rather than list out both in that function? It might make it more clear that this is what makes an infix an infix.

Just to be clear, the example in the original post isn’t about selecting from a fixed set of choices at all.

Drop the :choices '("fox" "kitten" "peregrine" "otter") and then above code can be used to enter multiple arbitrary values separated by ,, similar to your code above but much less painful to use than:

My suggestion is to have it so that if you click on the mount option, you enter a single value. If you click on it again, you enter the next value. If you enter an empty string, it pops the last value off of the list.

(Currently something seems to be broken on next (I made lots of huge changes today) so when you edit the existing multi-value, then you start with empty input. That's not how it is supposed to be and I will fix it soon. Your are supposed to get the old value and then can add new values or remove old ones, simply by editing the string (something like foo,bar,baz). As I said, you might want to wait until tomorrow evening.)

@tarsius - Great. As I said, the always-read slot seems to be what’s preventing re-picking an option. There are also issues with adding lists to the history for the option and I haven’t checked how it works with the save feature.

If you could post a mock up which does exactly what my toy example does that would be awesome.

Also to be clear, the idea above is not to edit the same string with some convention for joining values which leads to escaping issues. But rather to collect successive values. This last makes it easier to say validate each value on entry without having to parse the whole list.

I also think entering PATH=.,DEBUG=true feels weird when it’s rendered as -e PATH=. -e DEBUG=true and more so if you want to auto-quote or escape the values.

On reflection, updating a single string with a separator is exactly what the complaints are about. Currently people use the switch itself as a separator and I’m not sure using a comma improves the situation much. Having the multi-value use always-read semantics simply saves you from using the history to re-edit a value so that doesn’t seem like much of a win either.

I do think the behavior in the original example is much closer to what people naturally expect. The odd part is that it’s necessarily a stack but that only becomes an issue when editing the set of options and not when adding one. I.e. for the majority of usage it’s not noticeable. Even this oddness can be worked around by having the history available.

I am assuming this is about improving:

(transient-define-prefix docker-compose-exec ()
  "Transient for \"docker-compose exec\"."
  :man-page "docker-compose exec"
  ["Arguments"
   ("P" "Privileged" "--privileged")
   ("T" "Disable pseudo-tty" "-T")
   ("d" "Detach" "-d")
   ("e" "Env KEY=VAL" "-e " read-string)
   ("u" "User " "--user " read-string)
   ("w" "Workdir" "--workdir " read-string)]
  ["Actions"
   ("E" "Exec" docker-compose-run-action-with-command)])

and/or (the -f in):

(docker-utils-transient-define-prefix docker-container-logs ()
  "Transient for showing containers logs."
  :man-page "docker-container-logs"
  ["Arguments"
   ("f" "Follow" "-f")
   ("s" "Since" "--since " read-string)
   ("t" "Tail" "--tail " read-string)
   ("u" "Until" "--until " read-string)]
  [:description docker-utils-generic-actions-heading
   ("L" "Logs" docker-utils-generic-action-async)])

(Please correct me if I am wrong, pointing me to the suffixes that need improving.)

In the second case (which I assume is what #137 is about) I think the approach I have suggested above works perfect. This is very similar to the magit:-- suffix. I am working on improvements for that on the next branch so that each value that is read like foo.txt,bar.txt is returned paired like ("-f" "foo.txt" "-f" "bar.txt").

In the other case where the values are themselves KEY=VALUE pairs I agree that maybe we could come up with something better.

If you read the issues cited people are able to work around the issues they’ve raised so it’s less a question of functionality and more a question of least surprise.

Where you suggest typing foo.txt,bar.txt they have been typing foo.txt -f bar.txt and been getting the behavior they expect but the experience doesn’t feel smooth.

The experience doesn’t change much if you use a comma instead of -f. Also, there’s no reason the option itself doesn’t admit commas which means it would be parsed wrong if a comma was used as a separator. To fix this, you’d have to allow escaping of commas making the experience even further from expectations.

Re-selecting an option when you want another value lines up selection of options with the values entered without introducing friction from the processing tool.

Please do look at the other issues posted. I believe they line up with my interpretation.

FWIW, I think that the case is clearer with the -e option (as you call KEY=VALUE pairs) but the point is that using the comma is syntax not available to the underlying command. docker in this case.

We are talking past each other. I think you should write a complete implementation of the behavior you want (not just an example but something docker.el could use). You may have to define methods that you think should not be necessary or you might even have to patch transient.

Once that is done I can have a look and see whether I now agree that the UI that you propose is better than something based on completing-read-multiple.

If you don't want to do that, that's fine. I do have some ideas for alternative ways of picking values and maybe once I have implemented them (which may take a while) you like them and maybe they are very close to what you want. But currently we are just talking past each other and I don't think that that will change until one of us produces functioning, non-proof-of-concept code to show the other.

Yes, we are. I'm not sure why. I think I've been fairly explicit and direct and haven't felt the responses to be the same. I'm fast losing interest in this. I was only trying to offer an improvement to Silex's project. I'll move on to other things. I wish you luck.

Good luck to you too.

Linking for reference.

@gitonthescene, @tarsius: thanks for helping me figure out what needs to be done :heart:

@tarsius: basically some options are unique, some options can be repeated.

Right now there are 3 cases where this would be useful:

• docker run -e FOO=123 -e BAR=456 debian bash -c 'echo $FOO; echo $BAR', see https://github.com/Silex/docker.el/blob/master/docker-image.el#L266. At the moment to achieve this people press e then input FOO=123 -e BAR=456 which is not really ideal as you have to know the implementation detail.
• docker run -v /tmp:/tmp -v /usr:/usr debian bash (the -v mount flag can be repeated).
• docker-compose -f docker-compose.yml -f dev.yml run --rm foo bar, better explained in #137

@tarsius: I'm not sure what the best behavior would be to input repeatable options.

@gitonthescene suggested we press e then input FOO=123, then we press e again then input BAR=456, which sounds decent. If you want to remove the last item then press e and directly enter.

Another way could be you press e then input FOO=123,BAR=456 but the coma-separated way does not feel very natural compared to the console experience. It's also close to the current way of simply typing -e as the separator.

Something that would make sense according to how emacs works in other areas would be you press e, then the prompt is like Enter one env var then press enter (this repeats until you press C-M-x) and so you'd input FOO=123 and press enter then BAR=456 and press enter then KEKE=789 then you'd press C-M-x.

In all of these inputs I have no clue what the right behavior/keys would be if you want to clear/add to the current options.

This is likely something transient was not really designed for (usually options are not repeatable) so there's no obvious way of implementing it.

It will be a while until I get back to this. I am currently working on transient a lot to address various defects and add new features, which I consider more urgent than this. Among other things completing those things will make it easier to do "this".

No worries, don't hesitate to ask if this was unclear, I can make a simpler explanation.

@Silex - FWIW, my solution has the virtue that if you don't need to use a repeatable option more than once it behaves exactly the same as non-repeatable options. i.e. you don't need to any closing key combo like C-M-x. Not to mention that its implementation is straightforward.

@joshbax189 if you want to tackle this it's probably the next thing where people complain a lot (see #137).

See my previous comment about possible options, I think we first need to reflect on how the user experience would look like then try to implement it.

Saw this in transient's CHANGELOG:

The transient-options class now supports two types of options that can have multiple values: repeated option-value pairs and a final option that takes all remaining arguments as value. #154

Probably something worth exploring.

The date of that issue closing relative to when this conversation took place is curious.

Found this, shame there is no example.

https://github.com/positron-solutions/transient-showcase#multiple-instances

@positron-solutions (or @tarsius) can you maybe show us a rough draft of a sample?