Fix genspider --editor (#7683)

scrapy/commands/edit.py

@@ -1,12 +1,29 @@
-import argparse
+from __future__ import annotations
+
 import os
+import shlex
+import subprocess
 import sys
-from typing import Any, ClassVar
+from pathlib import Path
+from typing import TYPE_CHECKING, Any, ClassVar
 
 from scrapy.commands import ScrapyCommand
 from scrapy.exceptions import UsageError
 from scrapy.spiderloader import get_spider_loader
 
+if TYPE_CHECKING:
+    import argparse
+
+
+def _edit_file(editor: str, file_path: str | os.PathLike[str]) -> int:
+    """Open ``file_path`` with ``editor`` and return the editor exit code.
+
+    ``editor`` may include arguments (e.g. ``"code -w"``); it is split with
+    :func:`shlex.split` and the file is passed as a separate argument, so no
+    shell is involved.
+    """
+    return subprocess.call([*shlex.split(editor), os.fspath(file_path)])  # noqa: S603
+
 
 class Command(ScrapyCommand):
     requires_project = True
@@ -45,4 +62,4 @@ class Command(ScrapyCommand):
         sfile = sys.modules[spidercls.__module__].__file__
         assert sfile
         sfile = sfile.replace(".pyc", ".py")
-        self.exitcode = os.system(f'{editor} "{sfile}"')  # noqa: S605
+        self.exitcode = _edit_file(editor, Path(sfile))

scrapy/commands/genspider.py

@@ -1,6 +1,5 @@
 from __future__ import annotations
 
-import os
 import shutil
 import string
 from importlib import import_module
@@ -10,12 +9,14 @@ from urllib.parse import urlparse
 
 import scrapy
 from scrapy.commands import ScrapyCommand
+from scrapy.commands.edit import _edit_file
 from scrapy.exceptions import UsageError
 from scrapy.spiderloader import get_spider_loader
 from scrapy.utils.template import render_templatefile, string_camelcase
 
 if TYPE_CHECKING:
     import argparse
+    import os
 
 
 def sanitize_module_name(module_name: str) -> str:
@@ -118,9 +119,11 @@ class Command(ScrapyCommand):
 
         template_file = self._find_template(opts.template)
         if template_file:
-            self._genspider(module, name, url, opts.template, template_file)
+            spider_file = self._genspider(
+                module, name, url, opts.template, template_file
+            )
             if opts.edit:
-                self.exitcode = os.system(f'scrapy edit "{name}"')  # noqa: S605
+                self.exitcode = _edit_file(self.settings["EDITOR"], spider_file)
 
     def _generate_template_variables(
         self,
@@ -148,7 +151,7 @@ class Command(ScrapyCommand):
         url: str,
         template_name: str,
         template_file: str | os.PathLike[str],
-    ) -> None:
+    ) -> Path:
         """Generate the spider module, based on the given template"""
         assert self.settings is not None
         tvars = self._generate_template_variables(module, name, url, template_name)
@@ -168,6 +171,7 @@ class Command(ScrapyCommand):
         )
         if spiders_module:
             print(f"in module:\n  {spiders_module.__name__}.{module}")
+        return Path(spider_file)
 
     def _find_template(self, template: str) -> Path | None:
         template_file = Path(self.templates_dir, f"{template}.tmpl")

tests/test_command_genspider.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import re
+import sys
 from pathlib import Path
 
 import pytest
@@ -9,6 +10,13 @@ from tests.test_commands import TestProjectBase
 from tests.utils.cmdline import call, proc
 
 
+def write_recording_editor(editor: Path) -> None:
+    """Create an executable editor script that writes the path it is asked to
+    open (its last argument) into the file given as its first argument."""
+    editor.write_text('#!/bin/sh\nprintf "%s" "$2" > "$1"\n', encoding="utf-8")
+    editor.chmod(0o755)
+
+
 def find_in_file(filename: Path, regex: str) -> re.Match[str] | None:
     """Find first pattern occurrence in file"""
     pattern = re.compile(regex)
@@ -63,6 +71,28 @@ class TestGenspiderCommand(TestProjectBase):
         assert call("genspider", "--dump=basic", cwd=proj_path) == 0
         assert call("genspider", "-d", "basic", cwd=proj_path) == 0
 
+    @pytest.mark.skipif(
+        sys.platform == "win32", reason="requires a POSIX shell editor script"
+    )
+    def test_edit(self, proj_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
+        spider = proj_path / self.project_name / "spiders" / "example2.py"
+        edited = proj_path / "edited.txt"
+        editor = proj_path / "fake-editor.sh"
+        write_recording_editor(editor)
+        # The extra argument exercises shlex-splitting of the EDITOR value.
+        monkeypatch.setenv("EDITOR", f"{editor} {edited}")
+
+        returncode, _, err = proc(
+            "genspider", "--edit", "example2", "example2.com", cwd=proj_path
+        )
+
+        assert returncode == 0, err
+        assert "ModuleNotFoundError" not in err
+        assert spider.exists()
+        assert (proj_path / edited.read_text(encoding="utf-8")).resolve() == (
+            spider.resolve()
+        )
+
     def test_same_name_as_project(self, proj_path: Path) -> None:
         assert call("genspider", self.project_name, cwd=proj_path) == 2
         assert not (
@@ -168,6 +198,26 @@ class TestGenspiderStandaloneCommand:
         call("genspider", "example", "example.com", cwd=tmp_path)
         assert Path(tmp_path, "example.py").exists()
 
+    @pytest.mark.skipif(
+        sys.platform == "win32", reason="requires a POSIX shell editor script"
+    )
+    def test_edit(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
+        spider = tmp_path / "example.py"
+        edited = tmp_path / "edited.txt"
+        editor = tmp_path / "fake-editor.sh"
+        write_recording_editor(editor)
+        monkeypatch.setenv("EDITOR", f"{editor} {edited}")
+
+        returncode, _, err = proc(
+            "genspider", "--edit", "example", "example.com", cwd=tmp_path
+        )
+
+        assert returncode == 0, err
+        assert spider.exists()
+        assert (tmp_path / edited.read_text(encoding="utf-8")).resolve() == (
+            spider.resolve()
+        )
+
     @pytest.mark.parametrize("force", [True, False])
     def test_same_name_as_existing_file(self, force: bool, tmp_path: Path) -> None:
         file_name = "example"

tests/test_commands.py

@@ -2,6 +2,7 @@ from __future__ import annotations
 
 import argparse
 import json
+import sys
 from io import StringIO
 from shutil import copytree
 from typing import TYPE_CHECKING
@@ -420,6 +421,33 @@ class TestViewCommand:
         assert "URL using the Scrapy downloader and show its" in command.long_desc()
 
 
+class TestEditCommand(TestProjectBase):
+    @pytest.mark.skipif(
+        sys.platform == "win32", reason="requires a POSIX shell editor script"
+    )
+    def test_edit(self, proj_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
+        spider = proj_path / self.project_name / "spiders" / "example.py"
+        edited = proj_path / "edited.txt"
+        editor = proj_path / "fake-editor.sh"
+        # Records the file it is asked to open ($2) into the file given as $1.
+        editor.write_text('#!/bin/sh\nprintf "%s" "$2" > "$1"\n', encoding="utf-8")
+        editor.chmod(0o755)
+        monkeypatch.setenv("EDITOR", f"{editor} {edited}")
+
+        assert call("genspider", "example", "example.com", cwd=proj_path) == 0
+        returncode, _, err = proc("edit", "example", cwd=proj_path)
+
+        assert returncode == 0, err
+        assert (proj_path / edited.read_text(encoding="utf-8")).resolve() == (
+            spider.resolve()
+        )
+
+    def test_edit_spider_not_found(self, proj_path: Path) -> None:
+        returncode, _, err = proc("edit", "nonexistent", cwd=proj_path)
+        assert returncode == 1
+        assert "Spider not found: nonexistent" in err
+
+
 class TestHelpMessage(TestProjectBase):
     @pytest.mark.parametrize(
         "command",