Allow to send env as tuple

[tchalupnik]

Allow to send env as tuple in the process

[ronf]

When passing in a value yourself directly via the env argument, the value is supposed to already be a dictionary. Note the type signature in the documentation:

    :type env: `dict` with `str` keys and values

The only reason the check for List is present in that code is to handle the case where env was coming from a config file, via SSHConfig. While it works if you pass a List in via env yourself, that's undocumented and might not be supported in the future.

What is driving the need to pass in a tuple here instead of a dict?

[tchalupnik]

From the user's perspective, this is what VSC tells me and what mypy suggests

test.py:20: error: Argument "env" to "create_session" of "SSHClientConnection" has incompatible type "int"; expected "tuple[()] | Mapping[str, str] | Sequence[str] | None"  [arg-type]

for me it was weird you have DefTuple type but you don't allow the tuple to be passed so I thought it was a bug. At the same time you are right it is not documented so feel free to reject the PR or merge it.

[ronf]

Note that the only legal tuple value there is the empty tuple. As you said, this comes from DefTuple, but that's used as a way to internally track where the caller provided the env argument or not. If they didn't, the argument defaults to the empty tuple, and this signals that the env information should be pulled from the options argument:

        if env == ():
            env = self._options.env

This pattern is repeated on options to decide whether to pull the value from the config argument:

        self.env = cast(_Env, env if env != () else config.get('SetEnv'))

So, the internal type _Env needs to allow empty tuple as one of the options. This special meaning of the empty tuple is also potentially a good reason to not allow tuples here, as passing in an empty tuple would NOT mean the same thing as passing in an empty list. It would end up being handled as if env was not specified at all, reading the value from options.

That said, you're right that I could in theory tighten up this internal type. Currently, it is DefTuple(_Env), where _Env is:

_Env = Optional[Union[Mapping[str, str], Sequence[str]]]

I could replace Sequence here with List given the current implementation explicitly looks for it being of type list and treats all other types as needing to be a Mapping.

[tchalupnik]

Yes, you can either:

• Tighten up the allowed types
• Merge my code allowing the sequence as the argument

Feel free to decide :)

[ronf]

Sorry for the delay - I got caught up in some other work and missed that this was never resolved.

In addition to supporting lists of key=value strings, I've also added support for tuples as you requested, or other forms of sequences. It also now supports (key, value) tuples in addition to strings, and a previous check added support for byte strings in addition to Unicode, so that non-UTF-8 encoded data could be passed in environment variables. These changes are now available in commit b244bb33.