0
\$\begingroup\$

I want to have a config class with immutable lists. Is it ok to create it like this?

import os
from dataclasses import asdict, dataclass
from itertools import chain
from typing import Optional, Tuple, Mapping

ENV = Mapping[str, str]

@dataclass(frozen=True)
class Config:
    a_types: Tuple[str, ...]
    b_types: Tuple[str, ...]

    @classmethod
    def from_env(cls, env: Optional[ENV] = None) -> "Config":
        env = env or os.environ
        return cls(
            a_types=tuple(env["A_TYPES"].split(",")),
            b_types=tuple(env["B_TYPES"].split(",")),
        )

    @property
    def all(self) -> Tuple[str, ...]:
        return tuple(chain(*asdict(self).values()))
\$\endgroup\$
2
  • 4
    \$\begingroup\$ Please add the entire code: imports, constants, and so on :) And perhaps add some more context as to how you'll want to use this. \$\endgroup\$ Sep 22, 2021 at 10:32
  • \$\begingroup\$ (As presented, there's an obvious answer to Is it ok to code creating it like this?: Imagine not seeing this code for five years, and then needing to modify it.) \$\endgroup\$
    – greybeard
    Sep 23, 2021 at 7:37

1 Answer 1

1
\$\begingroup\$

I guess the idea is that "in production" configuration is taken from the environment, but in tests you can pass it in explicitly instead? If so, that seems fine.

If the intent is, rather, that some callers will pick up environment variables, while other callers ignore them, users might not be pleased. Nothing vexes me quite like a program blithely running with the wrong configuration, ignoring my attempts to configure it directly on the command line.

Other significant concerns:

  • There are packages for configuration. You should probably use one of them.

  • Whether this is OK depends more on what you're configuring than the code itself. Sometimes environment variables are a bad idea!

Nits:

  • This would throw if an environment variable is missing. Environment variables are usually optional with a reasonable default.

  • If you use environment variables, they should have a common prefix.

  • I would not make Config.all a @property, as a matter of style. As a user, if I see config.all() with parentheses, that tells me a method is being called and suggests the result is computed, not directly configurable. That's good information!

  • The implementation of all seems too clever; can't you write that as return self.a_types + self.b_types? (I'm imagining Config will evolve over time and collect all sorts of odd settings, not just lists that you want to concatenate.)

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.