One goal of samples was to never share code and let every sample be completely standalone. I don't think we want to violate this for a one-liner. Also, IMO we should be using the default shared path for all samples, not make a new config path.

Also, IMO we should be using the default shared path for all samples, not make a new config path.

As in the default config path? I'm a bit hesitant to do that. What if the user already has a config file there? I also think it would be less immediately obvious than having the TOML file right in the repo.

The alternative if we want to keep every sample standalone is to add a TOML file for every sample.

As in the default config path? I'm a bit hesitant to do that

Yes, it's how we did it in temporalio/samples-go#421 and I think samples need to show users how we expect them to use the API, which IMO is to use defaults unless there is a good reason not to. It makes sense for the sample to show the default/common/suggested/ideal/etc use of envconfig instead of encouraging them to make local config files.

What if the user already has a config file there?

Great, then the sample will work on that environment

Ah yes, I forgot - we already talked about this:

config = ClientConfigProfile.load()
config["address"] = "localhost:7233"
Client.connect(**config.to_client_connect_config())

I'll update accordingly, my mistake

It is annoying to have those three lines in every sample, I agree, but I can't think of a better way to achieve our goals of 1) no global/shared utilities across all samples, and 2) encourage use of the defaults as a user might in their code. Open to other suggestions here though or others' opinions in general.

Note, you may want this code:

config = ClientConfig.load_client_connect_config()
config.setdefault("target_host", "localhost:7233")
Client.connect(**config)

I think this is the more reasonable approach (load as normal, but set a default parameter value if it is not there). Many users may just remove the middle line when they don't want to have a default target host.

Resulted in:

    config_dict = ClientConfigProfile.load().to_dict()
    config_dict.setdefault("address", "localhost:7233")
    config = ClientConfigProfile.from_dict(config_dict)
    cl = await client.Client.connect(**config.to_client_connect_config())

ClientConfigProfile is a frozen class, and we can't mutate from load_client_connect_config() because it expects the address to already be set (will error otherwise).

It's not the cleanest, but I'd rather this than make ClientConfigProfile mutable or remove the address assertion.

ClientConfigProfile is a frozen class, and we can't mutate from load_client_connect_config()

ClientConfig.load_client_connect_config() doesn't return a ClientConfigProfile, it returns a dict. I think that simple 3-liner I had should work (create kwarg dict, set a key on it if not already set, use the kwarg dict).

Yes but load_client_connect_config() calls to_client_connect_config

which has a check on address:

def to_client_connect_config(self) -> ClientConnectConfig:
        """Create a `ClientConnectConfig` from this profile."""
        if not self.address:
            raise ValueError(
                "Configuration profile must contain an 'address' to be used for "
                "client connection"
            )

which will error in the general case (no address is provided).
So I went the roundabout way with the to_dict -> from_dict

👍 After off-PR discussion it was decided that this validation does not need to occur (Client.connect will error if it's missing the required param). We won't make some specific Python release for this minor change, but we may hold up this PR until a Python SDK release organically occurs with the change.

Where on the Python side is the default target_host set for this load?

The address is set in the added temporal.toml file, is that what you're asking?

Based on the other comment, if we are going with the default config file (which can be absent) to demonstrate ideal/suggested/simple/default/etc use of the envconfig API, then it may not have a target host here

Pedantic, but probably don't need this blank line in every sample