Home Original page

[py] integrate mypy type checking with Bazel by titusfortner · Pull Request #16958 · SeleniumHQ/selenium

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Use a standard Bazel mypy ruleset

Replace the custom run_mypy.py script and py_binary target with a standard,
community-maintained Bazel ruleset for mypy (e.g., from rules_python). This
change would enable more idiomatic and efficient per-target type checking,
leveraging Bazel's caching and parallelism.

Examples:

py/run_mypy.py [1-55] 
# Licensed to the Software Freedom Conservancy (SFC) under one
# or more contributor license agreements.  See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership.  The SFC licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License.  You may obtain a copy of the License at
#
#   http://www.apache.org/licenses/LICENSE-2.0
#

 ... (clipped 45 lines)
py/BUILD.bazel [651-663] 
py_binary(
    name = "mypy",
    srcs = ["run_mypy.py"],
    data = [
        "pyproject.toml",
        ":selenium",
    ],
    main = "run_mypy.py",
    deps = [
        ":selenium",

 ... (clipped 3 lines)

Solution Walkthrough:

Before:

# py/BUILD.bazel
py_binary(
    name = "mypy",
    srcs = ["run_mypy.py"],
    main = "run_mypy.py",
    deps = [":selenium", requirement("mypy")],
)

# py/run_mypy.py
def main():
    # ... setup logic ...
    # Run mypy on the entire 'selenium' package at once
    args = ["selenium", *sys.argv[1:]]
    stdout, stderr, exit_code = api.run(args)
    # ... handle output ...
    sys.exit(exit_code)

After:

# py/BUILD.bazel
# (The custom py_binary and run_mypy.py are removed)
# A standard mypy rule is used instead.
# This enables per-target caching and analysis.

load("@rules_python//python/integrations:mypy.bzl", "mypy_test")

mypy_test(
    name = "mypy_test",
    targets = [
        ":some_py_library",
        ":another_py_library",
        # ... granular targets instead of the whole package
    ],
)
Suggestion importance[1-10]: 9

__

Why: This is a high-impact architectural suggestion that correctly identifies that the PR's custom script approach misses key Bazel benefits like caching and parallelism, proposing a more idiomatic and efficient solution.

High
General
Ensure build before typechecking

In the GitHub Actions workflow, add needs: build to the typing job to ensure it
runs after the build job, consistent with other jobs.

.github/workflows/ci-python.yml [39-44]

 typing:
+  needs: build
   uses: ./.github/workflows/bazel.yml
   with:
     name: Type Checker
     run: bazel run //py:mypy
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This change aligns the typing job with the unit-tests job, which already depends on build, likely improving CI efficiency by reusing build artifacts and cache.

Medium
Lock Python version

In py/BUILD.bazel, add python_version = "PY3" to the mypy py_binary target to
lock the Python interpreter version.

py/BUILD.bazel [651-653]

 py_binary(
     name = "mypy",
+    python_version = "PY3",
     srcs = ["run_mypy.py"],
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: Explicitly setting python_version = "PY3" in the py_binary target improves build hermeticity and ensures the script runs with a consistent Python 3 interpreter.

Low
Possible issue
Add validation for directory existence

In py/run_mypy.py, add a check to verify that the target directory py_dir exists
before calling os.chdir() to prevent potential FileNotFoundError.

py/run_mypy.py [30-51]

 def main():
     # Find the workspace root - Bazel sets BUILD_WORKSPACE_DIRECTORY when using 'bazel run'
     workspace = os.environ.get("BUILD_WORKSPACE_DIRECTORY")
     if workspace:
         py_dir = os.path.join(workspace, "py")
     else:
         # Fallback for direct execution
         py_dir = os.path.dirname(os.path.abspath(__file__))
+
+    if not os.path.isdir(py_dir):
+        print(f"Error: Directory '{py_dir}' not found.", file=sys.stderr)
+        sys.exit(1)
 
     os.chdir(py_dir)
 
     # Run mypy on the selenium package
     # Configuration is read from pyproject.toml [tool.mypy] section
     args = ["selenium", *sys.argv[1:]]
     stdout, stderr, exit_code = api.run(args)
 
     if stdout:
         print(stdout, end="")
     if stderr:
         print(stderr, end="", file=sys.stderr)
 
     sys.exit(exit_code)
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion improves the script's robustness by adding a check for the existence of py_dir before changing to it, which is good practice for error handling.

Low
  • Update