Merge pull request #437 from Mic92/big-cleanup

Big cleanup

bin/nix-review

@@ -1,13 +1,11 @@
 #!/usr/bin/env python3
 # this script requires at least python 3.6!
-import os
 import sys
+from pathlib import Path
 
-sys.path.insert(
-    0, os.path.join(os.path.dirname(os.path.dirname(os.path.realpath(__file__))))
-)
+sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
 
-from nixpkgs_review import main  # NOQA
+from nixpkgs_review import main
 
 if __name__ == "__main__":
     main()

bin/nixpkgs-review

@@ -1,13 +1,11 @@
 #!/usr/bin/env python3
 # this script requires at least python 3.6!
-import os
 import sys
+from pathlib import Path
 
-sys.path.insert(
-    0, os.path.join(os.path.dirname(os.path.dirname(os.path.realpath(__file__))))
-)
+sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
 
-from nixpkgs_review import main  # NOQA
+from nixpkgs_review import main
 
 if __name__ == "__main__":
     main()

default.nix

@@ -24,10 +24,11 @@ python3Packages.buildPythonApplication {
     [
       python3Packages.setuptools
       python3Packages.pylint
-      glibcLocales
 
       # needed for interactive unittests
       python3Packages.pytest
+      python3Packages.pytest-xdist
+
       pkgs.nixVersions.stable or nix_2_4
       git
     ]

nixpkgs_review/builddir.py

@@ -1,5 +1,6 @@
 import os
 import signal
+import types
 from pathlib import Path
 from tempfile import TemporaryDirectory
 from typing import Any, Union
@@ -17,7 +18,12 @@ def handler(_sig: Any, _frame: Any) -> None:
 
         self.old_handler = signal.signal(signal.SIGINT, handler)
 
-    def __exit__(self, _type: Any, _value: Any, _traceback: Any) -> None:
+    def __exit__(
+        self,
+        _type: type[BaseException] | None,
+        _value: BaseException | None,
+        _traceback: types.TracebackType | None,
+    ) -> None:
         signal.signal(signal.SIGINT, self.old_handler)
 
 
@@ -39,9 +45,10 @@ def create_cache_directory(name: str) -> Union[Path, "TemporaryDirectory[str]"]:
             final_name = name if counter == 0 else f"{name}-{counter}"
             cache_home = xdg_cache.joinpath("nixpkgs-review", final_name)
             cache_home.mkdir(parents=True)
-            return cache_home
         except FileExistsError:
             counter += 1
+        else:
+            return cache_home
 
 
 class Builddir:
@@ -68,7 +75,12 @@ def __init__(self, name: str) -> None:
     def __enter__(self) -> "Builddir":
         return self
 
-    def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None:
+    def __exit__(
+        self,
+        exc_type: type[BaseException] | None,
+        exc_value: BaseException | None,
+        traceback: types.TracebackType | None,
+    ) -> None:
         os.environ.clear()
         os.environ.update(self.environ)
 

nixpkgs_review/buildenv.py

@@ -1,35 +1,34 @@
+import contextlib
 import os
 import sys
+import types
 from pathlib import Path
 from tempfile import NamedTemporaryFile
-from typing import Any
 
 from .utils import warn
 
 
-def find_nixpkgs_root() -> str | None:
+def find_nixpkgs_root() -> Path | None:
     prefix = ["."]
-    release_nix = ["nixos", "release.nix"]
     while True:
-        root_path = os.path.join(*prefix)
-        release_nix_path = os.path.join(root_path, *release_nix)
-        if os.path.exists(release_nix_path):
+        root_path = Path(*prefix)
+        release_nix_path = root_path / "nixos" / "release.nix"
+        if release_nix_path.exists():
             return root_path
-        if os.path.abspath(root_path) == "/":
+        if root_path == root_path.parent:
             return None
-        prefix.append("..")
+        root_path = root_path.parent
 
 
 class Buildenv:
     def __init__(self, allow_aliases: bool, extra_nixpkgs_config: str) -> None:
         if not (
             extra_nixpkgs_config.startswith("{") and extra_nixpkgs_config.endswith("}")
         ):
-            raise RuntimeError(
-                "--extra-nixpkgs-config must start with `{` and end with `}`"
-            )
+            msg = "--extra-nixpkgs-config must start with `{` and end with `}`"
+            raise RuntimeError(msg)
 
-        self.nixpkgs_config = NamedTemporaryFile(suffix=".nix")
+        self.nixpkgs_config = NamedTemporaryFile(suffix=".nix")  # noqa: SIM115
         self.nixpkgs_config.write(
             str.encode(
                 f"""{{
@@ -47,7 +46,7 @@ def __init__(self, allow_aliases: bool, extra_nixpkgs_config: str) -> None:
 
     def __enter__(self) -> Path:
         self.environ = os.environ.copy()
-        self.old_cwd = os.getcwd()
+        self.old_cwd = Path.cwd()
 
         root = find_nixpkgs_root()
         if root is None:
@@ -59,12 +58,15 @@ def __enter__(self) -> Path:
         os.environ["NIXPKGS_CONFIG"] = self.nixpkgs_config.name
         return Path(self.nixpkgs_config.name)
 
-    def __exit__(self, _type: Any, _value: Any, _traceback: Any) -> None:
+    def __exit__(
+        self,
+        _type: type[BaseException] | None,
+        _value: BaseException | None,
+        _traceback: types.TracebackType | None,
+    ) -> None:
         if self.old_cwd is not None:
-            try:
+            with contextlib.suppress(OSError):
                 os.chdir(self.old_cwd)
-            except OSError:  # could be deleted
-                pass
 
         if self.environ is not None:
             os.environ.clear()

nixpkgs_review/cli/__init__.py

@@ -9,7 +9,8 @@
 from shutil import which
 from typing import Any, cast
 
-from ..utils import nix_nom_tool
+from nixpkgs_review.utils import nix_nom_tool
+
 from .approve import approve_command
 from .comments import show_comments
 from .merge import merge_command
@@ -28,7 +29,8 @@ def regex_type(s: str) -> Pattern[str]:
     try:
         return re.compile(s)
     except re.error as e:
-        raise argparse.ArgumentTypeError(f"'{s}' is not a valid regex: {e}")
+        msg = f"'{s}' is not a valid regex: {e}"
+        raise argparse.ArgumentTypeError(msg) from e
 
 
 def pr_flags(
@@ -43,8 +45,8 @@ def pr_flags(
     )
     checkout_help = (
         "What to source checkout when building: "
-        + "`merge` will merge the pull request into the target branch, "
-        + "while `commit` will checkout pull request as the user has committed it"
+        "`merge` will merge the pull request into the target branch, "
+        "while `commit` will checkout pull request as the user has committed it"
     )
 
     pr_parser.add_argument(
@@ -133,7 +135,7 @@ def read_github_token() -> str | None:
     if token:
         return token
     try:
-        with open(hub_config_path(), encoding="utf-8") as f:
+        with hub_config_path().open() as f:
             for line in f:
                 # Allow substring match as hub uses yaml. Example string we match:
                 # " - oauth_token: ghp_abcdefghijklmnopqrstuvwxyzABCDEF1234\n"
@@ -145,7 +147,9 @@ def read_github_token() -> str | None:
     except OSError:
         pass
     if which("gh"):
-        r = subprocess.run(["gh", "auth", "token"], stdout=subprocess.PIPE, text=True)
+        r = subprocess.run(
+            ["gh", "auth", "token"], stdout=subprocess.PIPE, text=True, check=False
+        )
         if r.returncode == 0:
             return r.stdout.strip()
     return None

nixpkgs_review/cli/approve.py

@@ -1,6 +1,7 @@
 import argparse
 
-from ..github import GithubClient
+from nixpkgs_review.github import GithubClient
+
 from .utils import ensure_github_token, get_current_pr
 
 

nixpkgs_review/cli/comments.py

@@ -4,7 +4,8 @@
 from datetime import datetime
 from typing import Any
 
-from ..github import GithubClient
+from nixpkgs_review.github import GithubClient
+
 from .utils import ensure_github_token, get_current_pr
 
 
@@ -106,7 +107,8 @@ def from_json(data: dict[str, Any], comments: list[ReviewComment]) -> "Review":
 
 
 def parse_time(string: str) -> datetime:
-    return datetime.strptime(string, "%Y-%m-%dT%H:%M:%SZ")
+    # Should we care about timezone here? %z
+    return datetime.strptime(string, "%Y-%m-%dT%H:%M:%SZ")  # noqa: DTZ007
 
 
 def bold(text: str) -> str:
@@ -121,8 +123,7 @@ def get_comments(github_token: str, pr_num: int) -> list[Comment | Review]:
 
     comments: list[Comment | Review] = [Comment.from_json(pr)]
 
-    for comment in pr["comments"]["nodes"]:
-        comments.append(Comment.from_json(comment))
+    comments.extend(ReviewComment.from_json(c) for c in pr["reviews"]["nodes"])
 
     review_comments_by_ids: dict[str, ReviewComment] = {}
     for review in pr["reviews"]["nodes"]:

nixpkgs_review/cli/merge.py

@@ -1,6 +1,7 @@
 import argparse
 
-from ..github import GithubClient
+from nixpkgs_review.github import GithubClient
+
 from .utils import ensure_github_token, get_current_pr
 
 

nixpkgs_review/cli/post_result.py

@@ -3,8 +3,9 @@
 import sys
 from pathlib import Path
 
-from ..github import GithubClient
-from ..utils import warn
+from nixpkgs_review.github import GithubClient
+from nixpkgs_review.utils import warn
+
 from .utils import ensure_github_token
 
 
@@ -21,6 +22,5 @@ def post_result_command(args: argparse.Namespace) -> None:
         warn(f"Report not found in {report}. Are you in a nixpkgs-review nix-shell?")
         sys.exit(1)
 
-    with open(report, encoding="utf-8") as f:
-        report_text = f.read()
+    report_text = report.read_text()
     github_client.comment_issue(pr, report_text)

nixpkgs_review/cli/pr.py

@@ -2,17 +2,22 @@
 import re
 import sys
 from contextlib import ExitStack
-from pathlib import Path
+from typing import TYPE_CHECKING
+
+from nixpkgs_review.allow import AllowedFeatures
+from nixpkgs_review.builddir import Builddir
+from nixpkgs_review.buildenv import Buildenv
+from nixpkgs_review.errors import NixpkgsReviewError
+from nixpkgs_review.review import CheckoutOption, Review
+from nixpkgs_review.utils import System, warn
 
-from ..allow import AllowedFeatures
-from ..builddir import Builddir
-from ..buildenv import Buildenv
-from ..errors import NixpkgsReviewError
-from ..nix import Attr
-from ..review import CheckoutOption, Review
-from ..utils import System, warn
 from .utils import ensure_github_token
 
+if TYPE_CHECKING:
+    from pathlib import Path
+
+    from nixpkgs_review.nix import Attr
+
 
 def parse_pr_numbers(number_args: list[str]) -> list[int]:
     prs: list[int] = []

nixpkgs_review/cli/rev.py

@@ -1,10 +1,10 @@
 import argparse
 from pathlib import Path
 
-from ..allow import AllowedFeatures
-from ..buildenv import Buildenv
-from ..review import review_local_revision
-from ..utils import verify_commit_hash
+from nixpkgs_review.allow import AllowedFeatures
+from nixpkgs_review.buildenv import Buildenv
+from nixpkgs_review.review import review_local_revision
+from nixpkgs_review.utils import verify_commit_hash
 
 
 def rev_command(args: argparse.Namespace) -> Path:

nixpkgs_review/cli/utils.py

@@ -1,7 +1,7 @@
 import os
 import sys
 
-from ..utils import warn
+from nixpkgs_review.utils import warn
 
 
 def ensure_github_token(token: str | None) -> str:

nixpkgs_review/cli/wip.py

@@ -1,10 +1,10 @@
 import argparse
 from pathlib import Path
 
-from ..allow import AllowedFeatures
-from ..buildenv import Buildenv
-from ..review import review_local_revision
-from ..utils import verify_commit_hash
+from nixpkgs_review.allow import AllowedFeatures
+from nixpkgs_review.buildenv import Buildenv
+from nixpkgs_review.review import review_local_revision
+from nixpkgs_review.utils import verify_commit_hash
 
 
 def wip_command(args: argparse.Namespace) -> Path: