From 9aa3fa4aa07a8cb1d0052f288cd214ffe4eab12c Mon Sep 17 00:00:00 2001 From: Josh Temple <8672171+joshtemple@users.noreply.github.com> Date: Mon, 14 Mar 2022 20:55:38 -0400 Subject: [PATCH 1/9] Un-process dashes after parsing --- spectacles/cli.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/spectacles/cli.py b/spectacles/cli.py index e2db9a6a..8bfc46b1 100644 --- a/spectacles/cli.py +++ b/spectacles/cli.py @@ -200,13 +200,10 @@ def wrapper(*args, **kwargs): return wrapper -def preprocess_dashes(args: List[str]) -> List[str]: +def preprocess_dash(arg: str) -> str: """Replace any dashes with tildes, otherwise argparse will assume they're options""" pattern = re.compile(r"^-(?=([\w_\*]+/[\w_\*]+)|(\d+)$)") - processed = [] - for arg in args: - processed.append(pattern.sub("~", arg)) - return processed + return pattern.sub("~", arg) @handle_exceptions @@ -219,9 +216,13 @@ def main(): detail="The current Python version is %s." % platform.python_version(), ) - args = preprocess_dashes(sys.argv[1:]) + # Convert leading `-` to `~` so they don't break `parse_args` + args = [preprocess_dash(arg) for arg in sys.argv[1:]] parser = create_parser() args = parser.parse_args(args) + # Convert leading `~` back to `-` after `parse_args` + args.explores = [re.sub(r"^~", "-", arg) for arg in args.explores] + args.folders = [re.sub(r"^~", "-", arg) for arg in args.explores] branch = getattr(args, "branch", None) commit_ref = getattr(args, "commit_ref", None) From 0fb9f2af1a9e90e0ee8bed2e1e921af4a3f8d027 Mon Sep 17 00:00:00 2001 From: Josh Temple <8672171+joshtemple@users.noreply.github.com> Date: Mon, 14 Mar 2022 21:12:37 -0400 Subject: [PATCH 2/9] Update test to match function name change --- tests/test_cli.py | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index afccb205..e0a35597 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4,7 +4,7 @@ import requests from constants import ENV_VARS from utils import build_validation -from spectacles.cli import main, create_parser, handle_exceptions, preprocess_dashes +from spectacles.cli import main, create_parser, handle_exceptions, preprocess_dash from spectacles.exceptions import ( LookerApiError, SpectaclesException, @@ -376,20 +376,24 @@ def test_main_with_do_not_track(mock_tracking, mock_run_connect, env): def test_preprocess_dashes_with_folder_ids_should_work(): - args = preprocess_dashes(["--folders", "40", "25", "-41", "-1", "-344828", "3929"]) + args = [ + preprocess_dash(arg) + for arg in ["--folders", "40", "25", "-41", "-1", "-344828", "3929"] + ] assert args == ["--folders", "40", "25", "~41", "~1", "~344828", "3929"] def test_preprocess_dashes_with_model_explores_should_work(): - args = preprocess_dashes( - [ + args = [ + preprocess_dash(arg) + for arg in [ "--explores", "model_a/explore_a", "-model_b/explore_b", "model_c/explore_c", "-model_d/explore_d", ] - ) + ] assert args == [ "--explores", "model_a/explore_a", @@ -400,19 +404,22 @@ def test_preprocess_dashes_with_model_explores_should_work(): def test_preprocess_dashes_with_wildcards_should_work(): - args = preprocess_dashes( - [ + args = [ + preprocess_dash(arg) + for arg in ( "--explores", "*/explore_a", "-model_b/*", + "model-a/explore-a", "*/*", "-*/*", - ] - ) + ) + ] assert args == [ "--explores", "*/explore_a", "~model_b/*", + "model-a/explore-a", "*/*", "~*/*", ] From 5655b32ac9c278f8f588a5dbb9d75def11102133 Mon Sep 17 00:00:00 2001 From: Josh Temple <8672171+joshtemple@users.noreply.github.com> Date: Mon, 14 Mar 2022 21:24:24 -0400 Subject: [PATCH 3/9] Restore tildes to dashes after arg parsing --- spectacles/cli.py | 19 ++++++++++--------- spectacles/select.py | 2 +- spectacles/validators/content.py | 2 +- tests/test_cli.py | 23 ++++++++++++++++++++++- tests/test_select.py | 8 ++++---- 5 files changed, 38 insertions(+), 16 deletions(-) diff --git a/spectacles/cli.py b/spectacles/cli.py index 8bfc46b1..a9f5061f 100644 --- a/spectacles/cli.py +++ b/spectacles/cli.py @@ -202,8 +202,12 @@ def wrapper(*args, **kwargs): def preprocess_dash(arg: str) -> str: """Replace any dashes with tildes, otherwise argparse will assume they're options""" - pattern = re.compile(r"^-(?=([\w_\*]+/[\w_\*]+)|(\d+)$)") - return pattern.sub("~", arg) + return re.sub(r"^-(?=([\w_\*]+/[\w_\*]+)|(\d+)$)", "~", arg) + + +def restore_dash(arg: str) -> str: + """Convert leading tildes back to dashes.""" + return re.sub(r"^~", "-", arg) @handle_exceptions @@ -220,9 +224,6 @@ def main(): args = [preprocess_dash(arg) for arg in sys.argv[1:]] parser = create_parser() args = parser.parse_args(args) - # Convert leading `~` back to `-` after `parse_args` - args.explores = [re.sub(r"^~", "-", arg) for arg in args.explores] - args.folders = [re.sub(r"^~", "-", arg) for arg in args.explores] branch = getattr(args, "branch", None) commit_ref = getattr(args, "commit_ref", None) @@ -266,7 +267,7 @@ def main(): args.log_dir, args.project, ref, - args.explores, + [restore_dash(arg) for arg in args.explores], args.base_url, args.client_id, args.client_secret, @@ -285,7 +286,7 @@ def main(): run_assert( args.project, ref, - args.explores, + [restore_dash(arg) for arg in args.explores], args.base_url, args.client_id, args.client_secret, @@ -297,7 +298,7 @@ def main(): run_content( args.project, ref, - args.explores, + [restore_dash(arg) for arg in args.explores], args.base_url, args.client_id, args.client_secret, @@ -307,7 +308,7 @@ def main(): args.incremental, args.target, args.exclude_personal, - args.folders, + [restore_dash(arg) for arg in args.folders], ) elif args.command == "lookml": run_lookml( diff --git a/spectacles/select.py b/spectacles/select.py index cf44902b..f5b81660 100644 --- a/spectacles/select.py +++ b/spectacles/select.py @@ -31,7 +31,7 @@ def is_selected(model: str, explore: str, filters: List[str]) -> bool: included = None for f in filters: # If it matches an exclude, stop immediately - if f[0] == "~": + if f[0] == "-": if re.match(selector_to_pattern(f[1:]), test_string): return False elif included: diff --git a/spectacles/validators/content.py b/spectacles/validators/content.py index 57a0a363..390e1f9b 100644 --- a/spectacles/validators/content.py +++ b/spectacles/validators/content.py @@ -17,7 +17,7 @@ def __init__( exclude_folders = [] if folders: for folder_id in folders: - if folder_id.startswith("~"): + if folder_id.startswith("-"): exclude_folders.append(int(folder_id[1:])) else: include_folders.append(int(folder_id)) diff --git a/tests/test_cli.py b/tests/test_cli.py index e0a35597..ad83ef49 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -142,11 +142,14 @@ def test_parse_args_with_only_config_file(mock_parse_config, clean_env): "base_url": "BASE_URL_CONFIG", "client_id": "CLIENT_ID_CONFIG", "client_secret": "CLIENT_SECRET_CONFIG", + "project": "spectacles", + "explores": ["model_a/*", "-model_a/explore_b"], } - args = parser.parse_args(["connect", "--config-file", "config.yml"]) + args = parser.parse_args(["sql", "--config-file", "config.yml"]) assert args.base_url == "BASE_URL_CONFIG" assert args.client_id == "CLIENT_ID_CONFIG" assert args.client_secret == "CLIENT_SECRET_CONFIG" + assert args.explores == ["model_a/*", "-model_a/explore_b"] @patch("spectacles.cli.YamlConfigAction.parse_config") @@ -162,6 +165,24 @@ def test_parse_args_with_incomplete_config_file(mock_parse_config, clean_env, ca assert "the following arguments are required: --client-secret" in captured.err +@patch("spectacles.cli.run_sql") +@patch("spectacles.cli.YamlConfigAction.parse_config") +def test_config_file_explores_folders_processed_correctly( + mock_parse_config, mock_run_sql, clean_env +): + parser = create_parser() + mock_parse_config.return_value = { + "base_url": "BASE_URL_CONFIG", + "client_id": "CLIENT_ID_CONFIG", + "client_secret": "CLIENT_SECRET_CONFIG", + "project": "spectacles", + "explores": ["model_a/*", "-model_a/explore_b"], + } + with patch("sys.argv", ["spectacles", "sql", "--config-file", "config.yml"]): + main() + assert ["model_a/*", "-model_a/explore_b"] in mock_run_sql.call_args.args + + def test_parse_args_with_only_env_vars(env): parser = create_parser() args = parser.parse_args(["connect"]) diff --git a/tests/test_select.py b/tests/test_select.py index ac948c60..b62c7690 100644 --- a/tests/test_select.py +++ b/tests/test_select.py @@ -48,21 +48,21 @@ def test_select_wrong_explore_should_not_match(): assert not is_selected("model_a", "explore_a", ["model_a/explore_b"]) -@pytest.mark.parametrize("filters", permutations(["*/*", "~*/*"])) +@pytest.mark.parametrize("filters", permutations(["*/*", "-*/*"])) def test_exclude_wildcard_should_not_match(filters): assert not is_selected("model_a", "explore_a", filters) -@pytest.mark.parametrize("filters", permutations(["*/*", "~model_a/*"])) +@pytest.mark.parametrize("filters", permutations(["*/*", "-model_a/*"])) def test_exclude_model_wildcard_should_not_match(filters): assert not is_selected("model_a", "explore_a", filters) -@pytest.mark.parametrize("filters", permutations(["*/*", "~*/explore_a"])) +@pytest.mark.parametrize("filters", permutations(["*/*", "-*/explore_a"])) def test_exclude_explore_wildcard_should_not_match(filters): assert not is_selected("model_a", "explore_a", filters) -@pytest.mark.parametrize("filters", permutations(["*/*", "~model_a/explore_a"])) +@pytest.mark.parametrize("filters", permutations(["*/*", "-model_a/explore_a"])) def test_exclude_exact_model_and_explore_should_not_match(filters): assert not is_selected("model_a", "explore_a", filters) From e49052e16bf976c2bd0d3f7b98e12203675734d3 Mon Sep 17 00:00:00 2001 From: Josh Temple <8672171+joshtemple@users.noreply.github.com> Date: Mon, 14 Mar 2022 21:27:05 -0400 Subject: [PATCH 4/9] Prefer kwargs for explicitness --- spectacles/cli.py | 106 +++++++++++++++++++++++----------------------- 1 file changed, 53 insertions(+), 53 deletions(-) diff --git a/spectacles/cli.py b/spectacles/cli.py index a9f5061f..923d5488 100644 --- a/spectacles/cli.py +++ b/spectacles/cli.py @@ -256,71 +256,71 @@ def main(): if args.command == "connect": run_connect( - args.base_url, - args.client_id, - args.client_secret, - args.port, - args.api_version, + base_url=args.base_url, + client_id=args.client_id, + client_secret=args.client_secret, + port=args.port, + api_version=args.api_version, ) elif args.command == "sql": run_sql( - args.log_dir, - args.project, - ref, - [restore_dash(arg) for arg in args.explores], - args.base_url, - args.client_id, - args.client_secret, - args.port, - args.api_version, - args.fail_fast, - args.incremental, - args.target, - args.remote_reset, - args.concurrency, - args.profile, - args.runtime_threshold, - args.chunk_size, + logs_dir=args.log_dir, + project=args.project, + ref=ref, + explores=[restore_dash(arg) for arg in args.explores], + base_url=args.base_url, + client_id=args.client_id, + client_secret=args.client_secret, + port=args.port, + api_version=args.api_version, + fail_fast=args.fail_fast, + incremental=args.incremental, + target=args.target, + remote_reset=args.remote_reset, + concurrency=args.concurrency, + profile=args.profile, + runtime_threshold=args.runtime_threshold, + chunk_size=args.chunk_size, ) elif args.command == "assert": run_assert( - args.project, - ref, - [restore_dash(arg) for arg in args.explores], - args.base_url, - args.client_id, - args.client_secret, - args.port, - args.api_version, - args.remote_reset, + project=args.project, + ref=ref, + explores=[restore_dash(arg) for arg in args.explores], + base_url=args.base_url, + client_id=args.client_id, + client_secret=args.client_secret, + port=args.port, + api_version=args.api_version, + remote_reset=args.remote_reset, ) elif args.command == "content": run_content( - args.project, - ref, - [restore_dash(arg) for arg in args.explores], - args.base_url, - args.client_id, - args.client_secret, - args.port, - args.api_version, - args.remote_reset, - args.incremental, - args.target, - args.exclude_personal, - [restore_dash(arg) for arg in args.folders], + project=args.project, + ref=ref, + explores=[restore_dash(arg) for arg in args.explores], + base_url=args.base_url, + client_id=args.client_id, + client_secret=args.client_secret, + port=args.port, + api_version=args.api_version, + remote_reset=args.remote_reset, + incremental=args.incremental, + target=args.target, + exclude_personal=args.exclude_personal, + folders=[restore_dash(arg) for arg in args.folders], ) elif args.command == "lookml": run_lookml( - args.project, - ref, - args.base_url, - args.client_id, - args.client_secret, - args.port, - args.api_version, - args.remote_reset, - args.severity, + project=args.project, + ref=ref, + base_url=args.base_url, + client_id=args.client_id, + client_secret=args.client_secret, + port=args.port, + api_version=args.api_version, + remote_reset=args.remote_reset, + severity=args.severity, ) if not args.do_not_track: From 90461f305593156406065409901c6d186a64467c Mon Sep 17 00:00:00 2001 From: Josh Temple <8672171+joshtemple@users.noreply.github.com> Date: Mon, 14 Mar 2022 21:31:53 -0400 Subject: [PATCH 5/9] More tests --- tests/test_cli.py | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index ad83ef49..1cedad95 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -170,7 +170,6 @@ def test_parse_args_with_incomplete_config_file(mock_parse_config, clean_env, ca def test_config_file_explores_folders_processed_correctly( mock_parse_config, mock_run_sql, clean_env ): - parser = create_parser() mock_parse_config.return_value = { "base_url": "BASE_URL_CONFIG", "client_id": "CLIENT_ID_CONFIG", @@ -180,7 +179,37 @@ def test_config_file_explores_folders_processed_correctly( } with patch("sys.argv", ["spectacles", "sql", "--config-file", "config.yml"]): main() - assert ["model_a/*", "-model_a/explore_b"] in mock_run_sql.call_args.args + assert mock_run_sql.call_args.kwargs["explores"] == [ + "model_a/*", + "-model_a/explore_b", + ] + + +@patch("spectacles.cli.run_sql") +def test_cli_explores_folders_processed_correctly(mock_run_sql, clean_env): + with patch( + "sys.argv", + [ + "spectacles", + "sql", + "--base-url", + "BASE_URL", + "--client-id", + "CLIENT_ID", + "--client-secret", + "CLIENT_SECRET", + "--project", + "spectacles", + "--explores", + "model_a/*", + "-model_a/explore_b", + ], + ): + main() + assert mock_run_sql.call_args.kwargs["explores"] == [ + "model_a/*", + "-model_a/explore_b", + ] def test_parse_args_with_only_env_vars(env): From fed2760c72d8d4dd1ee8bc1c7988560d0c0e6b93 Mon Sep 17 00:00:00 2001 From: Josh Temple <8672171+joshtemple@users.noreply.github.com> Date: Mon, 14 Mar 2022 21:35:17 -0400 Subject: [PATCH 6/9] Appease flake8 --- spectacles/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spectacles/cli.py b/spectacles/cli.py index 923d5488..b678e94a 100644 --- a/spectacles/cli.py +++ b/spectacles/cli.py @@ -8,7 +8,7 @@ import argparse import logging import os -from typing import Callable, List +from typing import Callable from spectacles import __version__ from spectacles.runner import Runner from spectacles.client import LookerClient From b9ef0974e553740c7d1f854eedae0439ef007e43 Mon Sep 17 00:00:00 2001 From: Josh Temple <8672171+joshtemple@users.noreply.github.com> Date: Mon, 14 Mar 2022 21:41:24 -0400 Subject: [PATCH 7/9] Fix typo --- spectacles/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spectacles/cli.py b/spectacles/cli.py index b678e94a..58ff70bf 100644 --- a/spectacles/cli.py +++ b/spectacles/cli.py @@ -264,7 +264,7 @@ def main(): ) elif args.command == "sql": run_sql( - logs_dir=args.log_dir, + log_dir=args.log_dir, project=args.project, ref=ref, explores=[restore_dash(arg) for arg in args.explores], From 24bf8e0b913d443715d3da93f9c0c602f102d2c7 Mon Sep 17 00:00:00 2001 From: Josh Temple <8672171+joshtemple@users.noreply.github.com> Date: Mon, 14 Mar 2022 21:45:24 -0400 Subject: [PATCH 8/9] Fix more typos --- spectacles/cli.py | 6 +++--- tests/test_cli.py | 24 ++++++++++++------------ 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/spectacles/cli.py b/spectacles/cli.py index 58ff70bf..e7b261c3 100644 --- a/spectacles/cli.py +++ b/spectacles/cli.py @@ -267,7 +267,7 @@ def main(): log_dir=args.log_dir, project=args.project, ref=ref, - explores=[restore_dash(arg) for arg in args.explores], + filters=[restore_dash(arg) for arg in args.explores], base_url=args.base_url, client_id=args.client_id, client_secret=args.client_secret, @@ -286,7 +286,7 @@ def main(): run_assert( project=args.project, ref=ref, - explores=[restore_dash(arg) for arg in args.explores], + filters=[restore_dash(arg) for arg in args.explores], base_url=args.base_url, client_id=args.client_id, client_secret=args.client_secret, @@ -298,7 +298,7 @@ def main(): run_content( project=args.project, ref=ref, - explores=[restore_dash(arg) for arg in args.explores], + filters=[restore_dash(arg) for arg in args.explores], base_url=args.base_url, client_id=args.client_id, client_secret=args.client_secret, diff --git a/tests/test_cli.py b/tests/test_cli.py index 1cedad95..761d07c3 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -179,7 +179,7 @@ def test_config_file_explores_folders_processed_correctly( } with patch("sys.argv", ["spectacles", "sql", "--config-file", "config.yml"]): main() - assert mock_run_sql.call_args.kwargs["explores"] == [ + assert mock_run_sql.call_args.kwargs["filters"] == [ "model_a/*", "-model_a/explore_b", ] @@ -206,7 +206,7 @@ def test_cli_explores_folders_processed_correctly(mock_run_sql, clean_env): ], ): main() - assert mock_run_sql.call_args.kwargs["explores"] == [ + assert mock_run_sql.call_args.kwargs["filters"] == [ "model_a/*", "-model_a/explore_b", ] @@ -401,11 +401,11 @@ def test_main_with_connect(mock_tracking, mock_run_connect, env): ) mock_tracking.track_invocation_end.assert_called_once() mock_run_connect.assert_called_once_with( - "BASE_URL_ENV_VAR", # base_url - "CLIENT_ID_ENV_VAR", # client_id - "CLIENT_SECRET_ENV_VAR", # client_secret - 8080, # port - 3.1, # api_version + base_url="BASE_URL_ENV_VAR", + client_id="CLIENT_ID_ENV_VAR", + client_secret="CLIENT_SECRET_ENV_VAR", + port=8080, + api_version=3.1, ) @@ -417,11 +417,11 @@ def test_main_with_do_not_track(mock_tracking, mock_run_connect, env): mock_tracking.track_invocation_start.assert_not_called() mock_tracking.track_invocation_end.assert_not_called() mock_run_connect.assert_called_once_with( - "BASE_URL_ENV_VAR", # base_url - "CLIENT_ID_ENV_VAR", # client_id - "CLIENT_SECRET_ENV_VAR", # client_secret - 8080, # port - 3.1, # api_version + base_url="BASE_URL_ENV_VAR", + client_id="CLIENT_ID_ENV_VAR", + client_secret="CLIENT_SECRET_ENV_VAR", + port=8080, + api_version=3.1, ) From dd51e72dbd40f76858c3b9657ea68bb4d75ca6f7 Mon Sep 17 00:00:00 2001 From: Josh Temple <8672171+joshtemple@users.noreply.github.com> Date: Mon, 14 Mar 2022 21:53:24 -0400 Subject: [PATCH 9/9] Make tests work on Python 3.7 --- tests/test_cli.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 761d07c3..7115e73d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -179,7 +179,8 @@ def test_config_file_explores_folders_processed_correctly( } with patch("sys.argv", ["spectacles", "sql", "--config-file", "config.yml"]): main() - assert mock_run_sql.call_args.kwargs["filters"] == [ + + assert mock_run_sql.call_args[1]["filters"] == [ "model_a/*", "-model_a/explore_b", ] @@ -206,7 +207,7 @@ def test_cli_explores_folders_processed_correctly(mock_run_sql, clean_env): ], ): main() - assert mock_run_sql.call_args.kwargs["filters"] == [ + assert mock_run_sql.call_args[1]["filters"] == [ "model_a/*", "-model_a/explore_b", ]