Fix Ruff formatting when meta=none (#940)

Also, does a bunch of test refactoring to make future changes like this
easier by:

- Putting `Config` in a fixture, since most tests need it but don't care
what's in it
- Deleting more of the mocked tests, since they don't test much and are
hard to read

To get coverage back up, this means adding more functional e2e tests

---------

Co-authored-by: Dylan Anthony <[email protected]>

Author: dbanty

.changeset/fix_ruff_formatting_for_metanone.md

@@ -0,0 +1,14 @@
+---
+default: patch
+---
+
+# Fix Ruff formatting for `--meta=none`
+
+PR #940 fixes issue #939. Thanks @satwell!
+
+Due to the lack of `pyproject.toml`, Ruff was not getting configured properly when `--meta=none`.
+As a result, it didn't clean up common generation issues like duplicate imports, which would then cause errors from 
+linters.
+
+This is now fixed by changing the default `post_hook` to `ruff check . --fix --extend-select=I` when `--meta=none`.
+Using `generate --meta=none` should now be almost identical to the code generated by `update`.

.changeset/include_the_up_rule_for_generated_ruff_config.md

@@ -0,0 +1,8 @@
+---
+default: minor
+---
+
+# Include the `UP` rule for generated Ruff config
+
+This enables [pyupgrade-like improvements](https://docs.astral.sh/ruff/rules/#pyupgrade-up) which should replace some 
+`.format()` calls with f-strings.

.github/check_for_changes.py

@@ -149,11 +149,6 @@ jobs:
           pip install pdm
           python -m venv .venv
           pdm install
-      - name: Regenerate Integration Client
-        run: |
-          pdm run openapi-python-client update --url http://localhost:3000/openapi.json --config integration-tests-config.yaml --meta pdm
-      - name: Check for any file changes
-        run: python .github/check_for_changes.py
       - name: Cache Generated Client Dependencies
         uses: actions/cache@v3
         with:

.github/workflows/checks.yml

@@ -0,0 +1,2 @@
+post_hooks:
+  - echo "this should fail" && exit 1

end_to_end_tests/custom_post_hooks.config.yml

@@ -34,9 +34,7 @@ def _get_kwargs(
 
     _kwargs: Dict[str, Any] = {
         "method": "get",
-        "url": "/parameter-references/{path_param}".format(
-            path_param=path_param,
-        ),
+        "url": f"/parameter-references/{path_param}",
         "params": params,
         "cookies": cookies,
     }

end_to_end_tests/golden-record/my_test_api_client/api/parameter_references/get_parameter_references_path_param.py

@@ -21,9 +21,7 @@ def _get_kwargs(
 
     _kwargs: Dict[str, Any] = {
         "method": "delete",
-        "url": "/common_parameters_overriding/{param}".format(
-            param=param_path,
-        ),
+        "url": f"/common_parameters_overriding/{param_path}",
         "params": params,
     }
 

end_to_end_tests/golden-record/my_test_api_client/api/parameters/delete_common_parameters_overriding_param.py

@@ -21,9 +21,7 @@ def _get_kwargs(
 
     _kwargs: Dict[str, Any] = {
         "method": "get",
-        "url": "/common_parameters_overriding/{param}".format(
-            param=param_path,
-        ),
+        "url": f"/common_parameters_overriding/{param_path}",
         "params": params,
     }
 

end_to_end_tests/golden-record/my_test_api_client/api/parameters/get_common_parameters_overriding_param.py

@@ -31,9 +31,7 @@ def _get_kwargs(
 
     _kwargs: Dict[str, Any] = {
         "method": "get",
-        "url": "/same-name-multiple-locations/{param}".format(
-            param=param_path,
-        ),
+        "url": f"/same-name-multiple-locations/{param_path}",
         "params": params,
         "cookies": cookies,
     }

end_to_end_tests/golden-record/my_test_api_client/api/parameters/get_same_name_multiple_locations_param.py

@@ -16,12 +16,7 @@ def _get_kwargs(
 ) -> Dict[str, Any]:
     _kwargs: Dict[str, Any] = {
         "method": "get",
-        "url": "/multiple-path-parameters/{param4}/something/{param2}/{param1}/{param3}".format(
-            param4=param4,
-            param2=param2,
-            param1=param1,
-            param3=param3,
-        ),
+        "url": f"/multiple-path-parameters/{param4}/something/{param2}/{param1}/{param3}",
     }
 
     return _kwargs

end_to_end_tests/golden-record/my_test_api_client/api/parameters/multiple_path_parameters.py

@@ -21,5 +21,5 @@ requires = ["poetry-core>=1.0.0"]
 build-backend = "poetry.core.masonry.api"
 
 [tool.ruff]
-select = ["F", "I"]
+select = ["F", "I", "UP"]
 line-length = 120

end_to_end_tests/golden-record/pyproject.toml

@@ -19,5 +19,5 @@ requires = ["pdm-backend"]
 build-backend = "pdm.backend"
 
 [tool.ruff]
-select = ["F", "I"]
+select = ["F", "I", "UP"]
 line-length = 120

end_to_end_tests/metadata_snapshots/pdm.pyproject.toml

@@ -21,5 +21,5 @@ requires = ["poetry-core>=1.0.0"]
 build-backend = "poetry.core.masonry.api"
 
 [tool.ruff]
-select = ["F", "I"]
+select = ["F", "I", "UP"]
 line-length = 120

end_to_end_tests/metadata_snapshots/poetry.pyproject.toml

@@ -79,12 +79,11 @@ def regen_custom_template_golden_record():
     gr_path = Path(__file__).parent / "golden-record"
     tpl_gr_path = Path(__file__).parent / "custom-templates-golden-record"
 
-    output_path = Path(tempfile.mkdtemp())
+    output_path = Path.cwd() / "my-test-api-client"
     config_path = Path(__file__).parent / "config.yml"
 
     shutil.rmtree(tpl_gr_path, ignore_errors=True)
 
-    os.chdir(str(output_path.absolute()))
     result = runner.invoke(
         app,
         [
@@ -96,9 +95,8 @@ def regen_custom_template_golden_record():
     )
 
     if result.stdout:
-        generated_output_path = output_path / "my-test-api-client"
-        for f in generated_output_path.glob("**/*"):  # nb: works for Windows and Unix
-            relative_to_generated = f.relative_to(generated_output_path)
+        for f in output_path.glob("**/*"):  # nb: works for Windows and Unix
+            relative_to_generated = f.relative_to(output_path)
             gr_file = gr_path / relative_to_generated
             if not gr_file.exists():
                 print(f"{gr_file} does not exist, ignoring")

end_to_end_tests/regen_golden_record.py

@@ -21,5 +21,5 @@ requires = ["poetry-core>=1.0.0"]
 build-backend = "poetry.core.masonry.api"
 
 [tool.ruff]
-select = ["F", "I"]
+select = ["F", "I", "UP"]
 line-length = 120

end_to_end_tests/test-3-1-golden-record/pyproject.toml

@@ -28,9 +28,7 @@ def _get_kwargs(
 
     _kwargs: Dict[str, Any] = {
         "method": "post",
-        "url": "/const/{path}".format(
-            path=path,
-        ),
+        "url": f"/const/{path}",
         "params": params,
     }
 

end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/api/const/post_const_path.py

@@ -1,9 +1,10 @@
 import shutil
 from filecmp import cmpfiles, dircmp
 from pathlib import Path
-from typing import Dict, List, Optional, Tuple
+from typing import Dict, List, Optional, Set
 
 import pytest
+from click.testing import Result
 from typer.testing import CliRunner
 
 from openapi_python_client.cli import app
@@ -13,6 +14,7 @@ def _compare_directories(
     record: Path,
     test_subject: Path,
     expected_differences: Dict[Path, str],
+    expected_missing: Optional[Set[str]] = None,
     ignore: List[str] = None,
     depth=0,
 ):
@@ -28,7 +30,7 @@ def _compare_directories(
     first_printable = record.relative_to(Path.cwd())
     second_printable = test_subject.relative_to(Path.cwd())
     dc = dircmp(record, test_subject, ignore=[".ruff_cache", "__pycache__"] + (ignore or []))
-    missing_files = dc.left_only + dc.right_only
+    missing_files = set(dc.left_only + dc.right_only) - (expected_missing or set())
     if missing_files:
         pytest.fail(
             f"{first_printable} or {second_printable} was missing: {missing_files}",
@@ -77,21 +79,23 @@ def _compare_directories(
 def run_e2e_test(
     openapi_document: str,
     extra_args: List[str],
-    expected_differences: Dict[Path, str],
+    expected_differences: Optional[Dict[Path, str]] = None,
     golden_record_path: str = "golden-record",
     output_path: str = "my-test-api-client",
-):
+    expected_missing: Optional[Set[str]] = None,
+) -> Result:
     output_path = Path.cwd() / output_path
     shutil.rmtree(output_path, ignore_errors=True)
-    generate(extra_args, openapi_document)
+    result = generate(extra_args, openapi_document)
     gr_path = Path(__file__).parent / golden_record_path
 
+    expected_differences = expected_differences or {}
     # Use absolute paths for expected differences for easier comparisons
     expected_differences = {
         output_path.joinpath(key): value for key, value in expected_differences.items()
     }
     _compare_directories(
-        gr_path, output_path, expected_differences=expected_differences
+        gr_path, output_path, expected_differences=expected_differences, expected_missing=expected_missing
     )
 
     import mypy.api
@@ -100,19 +104,31 @@ def run_e2e_test(
     assert status == 0, f"Type checking client failed: {out}"
 
     shutil.rmtree(output_path)
+    return result
 
 
-def generate(extra_args: Optional[List[str]], openapi_document: str):
+def generate(extra_args: Optional[List[str]], openapi_document: str) -> Result:
+    """Generate a client from an OpenAPI document and return the path to the generated code"""
+    _run_command("generate", extra_args, openapi_document)
+
+
+def _run_command(command: str, extra_args: Optional[List[str]] = None, openapi_document: Optional[str] = None, url: Optional[str] = None, config_path: Optional[Path] = None) -> Result:
     """Generate a client from an OpenAPI document and return the path to the generated code"""
     runner = CliRunner()
-    openapi_path = Path(__file__).parent / openapi_document
-    config_path = Path(__file__).parent / "config.yml"
-    args = ["generate", f"--config={config_path}", f"--path={openapi_path}"]
+    if openapi_document is not None:
+        openapi_path = Path(__file__).parent / openapi_document
+        source_arg = f"--path={openapi_path}"
+    else:
+        source_arg = f"--url={url}"
+    config_path = config_path or (Path(__file__).parent / "config.yml")
+    args = [command, f"--config={config_path}", source_arg]
     if extra_args:
         args.extend(extra_args)
     result = runner.invoke(app, args)
     if result.exit_code != 0:
-        raise result.exception
+        raise Exception(result.stdout)
+    return result
+
 
 def test_baseline_end_to_end_3_0():
     run_e2e_test("baseline_openapi_3.0.json", [], {})
@@ -147,19 +163,22 @@ def test_meta(meta: str, generated_file: Optional[str], expected_file: Optional[
 
     if generated_file and expected_file:
         assert (output_path / generated_file).exists()
-        assert (output_path / generated_file).read_text() == (Path(__file__).parent / "metadata_snapshots" / expected_file).read_text()
+        assert (
+            (output_path / generated_file).read_text() ==
+            (Path(__file__).parent / "metadata_snapshots" / expected_file).read_text()
+        )
 
     shutil.rmtree(output_path)
 
 
-def test_no_meta():
-    output_path = Path.cwd() / "test_3_1_features_client"
-    shutil.rmtree(output_path, ignore_errors=True)
-    generate([f"--meta=none"], "3.1_specific.openapi.yaml")
-
-    assert output_path.exists()  # Has a different name than with-meta generation
-    assert (output_path / "__init__.py").exists()
-    shutil.rmtree(output_path)
+def test_none_meta():
+    run_e2e_test(
+        "3.1_specific.openapi.yaml",
+        ["--meta=none"],
+        golden_record_path="test-3-1-golden-record/test_3_1_features_client",
+        output_path="test_3_1_features_client",
+        expected_missing={"py.typed"},
+    )
 
 
 def test_custom_templates():
@@ -194,3 +213,76 @@ def test_custom_templates():
         extra_args=["--custom-template-path=end_to_end_tests/test_custom_templates/"],
         expected_differences=expected_differences,
     )
+
+
+@pytest.mark.parametrize(
+    "command", ("generate", "update")
+)
+def test_bad_url(command: str):
+    runner = CliRunner()
+    result = runner.invoke(app, [command, "--url=not_a_url"])
+    assert result.exit_code == 1
+    assert "Could not get OpenAPI document from provided URL" in result.stdout
+
+
+def test_custom_post_hooks():
+    shutil.rmtree(Path.cwd() / "my-test-api-client", ignore_errors=True)
+    runner = CliRunner()
+    openapi_document = Path(__file__).parent / "baseline_openapi_3.0.json"
+    config_path = Path(__file__).parent / "custom_post_hooks.config.yml"
+    result = runner.invoke(app, ["generate", f"--path={openapi_document}", f"--config={config_path}"])
+    assert result.exit_code == 1
+    assert "this should fail" in result.stdout
+    shutil.rmtree(Path.cwd() / "my-test-api-client", ignore_errors=True)
+
+
+def test_generate_dir_already_exists():
+    project_dir = Path.cwd() / "my-test-api-client"
+    if not project_dir.exists():
+        project_dir.mkdir()
+    runner = CliRunner()
+    openapi_document = Path(__file__).parent / "baseline_openapi_3.0.json"
+    result = runner.invoke(app, ["generate", f"--path={openapi_document}"])
+    assert result.exit_code == 1
+    assert "Directory already exists" in result.stdout
+    shutil.rmtree(Path.cwd() / "my-test-api-client", ignore_errors=True)
+
+
+def test_update_dir_not_found():
+    project_dir = Path.cwd() / "my-test-api-client"
+    shutil.rmtree(project_dir, ignore_errors=True)
+    runner = CliRunner()
+    openapi_document = Path(__file__).parent / "baseline_openapi_3.0.json"
+    result = runner.invoke(app, ["update", f"--path={openapi_document}"])
+    assert result.exit_code == 1
+    assert str(project_dir) in result.stdout
+
+
+@pytest.mark.parametrize(
+    ("file_name", "content", "expected_error"),
+    (
+        ("invalid_openapi.yaml", "not a valid openapi document", "Failed to parse OpenAPI document"),
+        ("invalid_json.json", "Invalid JSON", "Invalid JSON"),
+        ("invalid_yaml.yaml", "{", "Invalid YAML"),
+    ),
+    ids=("invalid_openapi", "invalid_json", "invalid_yaml")
+)
+def test_invalid_openapi_document(file_name, content, expected_error):
+    runner = CliRunner()
+    openapi_document = Path.cwd() / file_name
+    openapi_document.write_text(content)
+    result = runner.invoke(app, ["generate", f"--path={openapi_document}"])
+    assert result.exit_code == 1
+    assert expected_error in result.stdout
+    openapi_document.unlink()
+
+
+def test_update_integration_tests():
+    url = "https://raw.githubusercontent.com/openapi-generators/openapi-test-server/main/openapi.json"
+    source_path = Path(__file__).parent.parent / "integration-tests"
+    project_path = Path.cwd() / "integration-tests"
+    if source_path != project_path:  # Just in case someone runs this from root dir
+        shutil.copytree(source_path, project_path)
+    config_path = project_path / "config.yaml"
+    _run_command("update", url=url, config_path=config_path)
+    _compare_directories(source_path, project_path, expected_differences={})

end_to_end_tests/test_end_to_end.py

@@ -0,0 +1,5 @@
+project_name_override: integration-tests
+post_hooks:
+  - ruff check . --fix
+  - ruff format .
+  - mypy . --strict