From b8f190ab33bb5119941f3c03546cd84b417d6e33 Mon Sep 17 00:00:00 2001 From: tlento Date: Mon, 14 Aug 2023 22:49:06 -0700 Subject: [PATCH] Remove EnvironmentVariable helper class This class is doing a whole lot to accomplish very little. In updating the sql engine url environment variable access I noticed the old EnvironmentVariable helper was hardly being used at all anymore. Since most of its usage has disappeared into the sands of time, we remove the last remaining calls - and the class itself - here. --- metricflow/configuration/__init__.py | 0 metricflow/configuration/env_var.py | 93 ---------------------- metricflow/test/fixtures/setup_fixtures.py | 12 ++- metricflow/test/generate_snapshots.py | 10 +-- 4 files changed, 13 insertions(+), 102 deletions(-) delete mode 100644 metricflow/configuration/__init__.py delete mode 100644 metricflow/configuration/env_var.py diff --git a/metricflow/configuration/__init__.py b/metricflow/configuration/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/metricflow/configuration/env_var.py b/metricflow/configuration/env_var.py deleted file mode 100644 index 44ff2be4f4..0000000000 --- a/metricflow/configuration/env_var.py +++ /dev/null @@ -1,93 +0,0 @@ -from __future__ import annotations - -import os -from typing import Optional - - -class EnvironmentVariable: - """Represents an environment variable and provides a way to retrieve the associated value.""" - - def __init__(self, name: str, default_value: Optional[str] = None): - """Constructor. - - Args: - name: The name of the environment variable that this represents. - default_value: The default value to return if this environment variable is not set. - """ - self._name = name - self._default_value = default_value - - def exists(self) -> bool: - """Returns true if the this environment variable exists / is set.""" - return os.getenv(self._name) is not None - - def get(self) -> str: - """Returns the value of this environment variable as a string. - - Returns the default value if not defined. If the environment variable is not defined and no default was - specified, this throws an exception. - """ - value = os.getenv(self._name) - if value is None and self._default_value is not None: - return self._default_value - if value is None: - raise RuntimeError(f"Environment variable {self._name} is not defined.") - return value - - def get_optional(self) -> Optional[str]: - """Returns value of env variable if it exists or None if it doesn't.""" - return os.getenv(self._name) - - def get_int(self) -> int: - """Returns value of this environment variable as an integer. - - Returns the default value if not defined. If the environment variable is not defined and no default was - specified, this throws an exception. - """ - value = os.environ.get(self._name) - if value is None and self._default_value is not None: - try: - return int(self._default_value) - except ValueError as e: - raise ValueError( - f"Default value for {self._name} is {self._default_value}, which is not an int." - ) from e - if value is None: - raise RuntimeError(f"Environment variable {self._name} is not defined.") - try: - return int(value) - except ValueError as e: - raise ValueError(f"Environment variable {self._name} is {self._default_value}, which is not an int.") from e - - @staticmethod - def _is_valid_bool(bool_str: str) -> bool: - """Checks if the given string can be converted into a bool.""" - return bool_str.lower() == "true" or bool_str.lower() == "false" - - def get_bool(self) -> bool: - """Returns the value of this environment variable as a bool. - - Returns the default value if not defined. If the environment variable is not defined and no default was - specified, this throws an exception. - """ - value = os.environ.get(self._name) - - if value is None: - if self._default_value is None: - raise RuntimeError(f"Environment variable {self._name} is not defined and has no default.") - - if not EnvironmentVariable._is_valid_bool(self._default_value): - raise ValueError( - f"Default value for {self._name} is {self._default_value}, which is not true or false." - ) - - return self._default_value.lower() == "true" - - if not EnvironmentVariable._is_valid_bool(value): - raise ValueError(f"Value for {self._name} is {value}, which is not true or false.") - - return value.lower() == "true" - - @property - def name(self) -> str: # noqa: D - return self._name diff --git a/metricflow/test/fixtures/setup_fixtures.py b/metricflow/test/fixtures/setup_fixtures.py index 9dd234050e..535dc21426 100644 --- a/metricflow/test/fixtures/setup_fixtures.py +++ b/metricflow/test/fixtures/setup_fixtures.py @@ -12,7 +12,6 @@ from _pytest.fixtures import FixtureRequest from sqlalchemy.engine import make_url -from metricflow.configuration.env_var import EnvironmentVariable from metricflow.random_id import random_id from metricflow.test.fixtures.sql_clients.common_client import SqlDialect from metricflow.test.table_snapshot.table_snapshots import SqlTableSnapshotHash, SqlTableSnapshotRepository @@ -154,8 +153,15 @@ def warn_user_about_slow_tests_without_parallelism( # noqa: D request: FixtureRequest, mf_test_session_state: MetricFlowTestSessionState, ) -> None: - worker_count_env_var = EnvironmentVariable("PYTEST_XDIST_WORKER_COUNT", "1") - num_workers = worker_count_env_var.get_int() + worker_count_env_var = os.environ.get("PYTEST_XDIST_WORKER_COUNT", "1") + try: + num_workers = int(worker_count_env_var) + except ValueError as e: + raise ValueError( + f"Could not convert environment variable PYTEST_XDIST_WORKER_COUNT to int! " + f"Value in environ was: {worker_count_env_var}" + ) from e + num_items = len(request.session.items) dialect = dialect_from_url(mf_test_session_state.sql_engine_url) diff --git a/metricflow/test/generate_snapshots.py b/metricflow/test/generate_snapshots.py index e6594f6612..7a9c4ead29 100644 --- a/metricflow/test/generate_snapshots.py +++ b/metricflow/test/generate_snapshots.py @@ -43,7 +43,6 @@ from dbt_semantic_interfaces.implementations.base import FrozenBaseModel from dbt_semantic_interfaces.pretty_print import pformat_big_objects -from metricflow.configuration.env_var import EnvironmentVariable from metricflow.protocols.sql_client import SqlEngine logger = logging.getLogger(__name__) @@ -162,15 +161,14 @@ def run_cli() -> None: # noqa: D dev_format = "%(asctime)s %(levelname)s %(filename)s:%(lineno)d [%(threadName)s] - %(message)s" logging.basicConfig(level=logging.INFO, format=dev_format) - credential_sets_json_env_var = EnvironmentVariable("MF_TEST_ENGINE_CREDENTIAL_SETS") - if credential_sets_json_env_var.get_optional() is None: + credential_sets_json_str = os.environ.get("MF_TEST_ENGINE_CREDENTIAL_SETS") + if credential_sets_json_str is None: raise ValueError( - f"Environment variable: {credential_sets_json_env_var.name} has not been set. Please see the comment in " + f"Environment variable: MF_TEST_ENGINE_CREDENTIAL_SETS has not been set. Please see the comment in " f"{__file__} for details on how to set it." ) - credentials_sets_json_str = credential_sets_json_env_var.get() - credential_sets = MetricFlowTestCredentialSetForAllEngines.parse_raw(credentials_sets_json_str) + credential_sets = MetricFlowTestCredentialSetForAllEngines.parse_raw(credential_sets_json_str) logger.info( f"Running the following tests to generate snapshots:\n{pformat_big_objects(SNAPSHOT_GENERATING_TEST_FILES)}"