Re: Review wanted for method for accessing Mock chroots

Tim Landscheidt <[email protected]> writes:

> Hi,

Hi Tim,

> Is there anything blatantly wrong/not backward-compatible/
> not foreward-compatible about it?  I saved on (customizable)
> variables as it is hard to predict what other users might
> want/need to change.  Any advice is appreciated.

Some few comments below.

However: I don't know Mock chroots in detail, but it looks to me like it
is something what could live in tramp-container.el. What do you think a bout?

Here are some few comments from roughly scanning the code:

> ;;; mock-tramp.el --- TRAMP integration for Mock chroots  -*- lexical-binding: t; -*-

Tramp is spelled out “Tramp”. See the manual.

> ;; Package-Requires: ((tramp "2.7.1-pre"))

Why "2.7.1-pre"? This isn't a released version; I would depend on "2.7.1".

> ;;;###autoload
> (defcustom mock-tramp-method "mock"
>   "TRAMP method to connect to Mock chroots."
>   :type 'string
>   :group 'mock-tramp)

You can keep the :group out; the last declared defgroup is taken by
default.

> ;;;###autoload
> (defun mock-tramp--list-chroots (directory)
>   "Return a list of chroots defined in DIRECTORY.

Why ;;;###autoload?

> ;;;###autoload
> (with-eval-after-load 'tramp
>   (add-to-list 'tramp-methods

Same here.

>                  (tramp-login-program "mock")

I would declare and use `mock-tramp-program'.

>                  (tramp-login-args (("-r") ("%h")
>                                     ("--shell"
>                                      "--"
>                                      "/usr/bin/env"
>                                      "PROMPT_COMMAND="
>                                      "/bin/sh"

I would use `tramp-default-remote-shell'.

>                                      "-l")))
>                  (tramp-remote-shell "/bin/sh")

Same here.

> TIA,
> Tim

Best regards, Michael.