python-code-review

name	python-code-review
description	Performs comprehensive code reviews for Python files following PEP 8 and Google Python Style Guide standards. Checks code quality, best practices, security, performance, maintainability, and style compliance. Use when reviewing Python code or when asked to check, audit, or improve Python code quality.

Python Code Review (PEP 8 + Google Style Guide)

Perform systematic code reviews of Python files following PEP 8 and Google Python Style Guide standards.

Review Philosophy

"Code is read much more often than it is written." - Guido van Rossum

Key Principle: A foolish consistency is the hobgoblin of little minds. Consistency within a project is more important than rigid adherence to rules. When in doubt, prioritize:

1. Consistency within one function/module (most important)
2. Consistency within the project
3. Consistency with PEP 8/Google Style Guide

Know when to be inconsistent:

• When applying the guideline makes code less readable
• To match surrounding code style (but consider refactoring)
• When code predates the guideline
• For backwards compatibility

Review Process

When reviewing Python code:

1. Read the entire file to understand its purpose, structure, and context

2. Analyze against these standards in order of priority:

1. Style & Formatting (PEP 8)

Line Length

• Maximum 80 characters (Google)
• Docstrings/comments: 72 characters max
• Use implicit line continuation (parentheses/brackets) over backslashes

Indentation

• Always 4 spaces per level, never tabs
• Continuation lines: align vertically or use 4-space hanging indent
• Closing brackets: align under first non-whitespace or under opening bracket

Blank Lines

• 2 blank lines between top-level functions/classes
• 1 blank line between methods
• Sparingly within functions for logical sections
• No blank line after def line

Whitespace Rules

• No whitespace inside parentheses/brackets/braces: spam(ham[1], {eggs: 2})
• No whitespace before comma/semicolon/colon (except in slices)
• No whitespace before function call parentheses: spam(1) not spam (1)
• No whitespace before indexing brackets: dct['key'] not dct ['key']
• Single space around binary operators: i = i + 1
• No spaces around = in keyword args: complex(real, imag=0.0)
• BUT use spaces when combining annotation + default: def munge(input: AnyStr = None)
• Don't align operators vertically across lines (maintenance burden)

String Quotes

• Be consistent: pick ' or " and stick with it in a file
• Use the other quote to avoid backslashes: "He said 'hello'"
• Always use """ for docstrings (never ''')

Trailing Commas

• Recommended for multi-line structures when closing bracket is on new line
• Mandatory for single-element tuples: FILES = ('setup.cfg',)
• Not on same line as closing bracket (except singleton tuples)

2. Imports (PEP 8 + Google)

Import Order (with blank line between groups):

1. from __future__ imports
2. Standard library imports
3. Third-party imports
4. Local application/library imports

Import Style

• Separate lines: import os and import sys (not import os, sys)
• Exception: OK to import multiple items from one module: from subprocess import Popen, PIPE
• Exception: Typing imports: from typing import Any, NewType
• Use absolute imports (recommended): import mypkg.sibling
• Relative imports acceptable for complex packages: from . import sibling
• Never use wildcard imports: from module import *
• Import full package paths (Google): from doctor.who import jodie not import jodie

Import Format

• import x for packages and modules
• from x import y where x is package prefix, y is module name
• from x import y as z if y conflicts or is inconveniently long
• import y as z only for standard abbreviations: import numpy as np

Module Dunders

• After module docstring, before imports (except __future__)
• Order: __all__, __version__, __author__, etc.

3. Naming Conventions (PEP 8 + Google)

Type	Convention	Examples
Modules	lower_with_under.py	my_module.py
Packages	lower_with_under	my_package
Classes	CapWords	MyClass, HTTPServerError
Exceptions	CapWords + Error suffix	ValueError, ConnectionError
Functions	lower_with_under()	my_function()
Methods	lower_with_under()	my_method()
Constants	CAPS_WITH_UNDER	MAX_OVERFLOW, TOTAL
Global/Class Variables	lower_with_under	global_var
Instance Variables	lower_with_under	instance_var
Parameters	lower_with_under	function_param
Local Variables	lower_with_under	local_var
Type Variables	_T, _P (leading underscore)	_T = TypeVar("_T")

Special Prefixes/Suffixes

• _single_leading: weak "internal use" indicator (not imported by from M import *)
• single_trailing_: avoid keyword conflicts (class_)
• __double_leading: name mangling in classes (discouraged by Google - impacts testability)
• __double_leading_and_trailing__: magic methods (never invent these)

Names to Avoid

• Never use l (lowercase L), O (uppercase o), I (uppercase i) as single-char names
• No dashes in any package/module name
• Avoid abbreviations unless well-known
• No offensive terms
• No needless type info: id_to_name_dict → id_to_name

Descriptive Names

• Names should be descriptive and clear
• Descriptiveness proportional to scope (wider scope = more descriptive)
• Single-char names OK for: counters (i, j, k), exceptions (e), file handles (f), type vars (_T, _P)
• Avoid vague names: thing, stuff, data (without context)

4. Documentation (PEP 257 + Google)

Module Docstrings (required)

"""One-line summary ending with period.

Longer description of the module or program. May include usage
examples, exported classes/functions, etc.

Typical usage example:
  foo = ClassFoo()
  bar = foo.function_bar()
"""

Function/Method Docstrings (required for complex/public functions)

def fetch_data(
    table: str,
    keys: Sequence[str],
    require_all: bool = False,
) -> Mapping[str, tuple[str, ...]]:
    """Fetches rows from database.

    Retrieves rows pertaining to given keys. Longer description
    of what the function does and any important details.

    Args:
        table: Name of the database table.
        keys: List of row keys to fetch. Strings will be UTF-8 encoded.
        require_all: If True, only return rows with all keys present.

    Returns:
        Dict mapping keys to row data. Each row is a tuple of strings.
        Returns empty dict if no rows found.

    Raises:
        IOError: Error accessing the database.
        ValueError: Invalid table name provided.
    """

Docstring Sections (Google style):

• Summary line: One physical line (≤80 chars), ends with period
• Blank line after summary (if more content follows)
• Args: Describe each parameter (with type if not annotated)
• Returns: Describe return value (omit for None, generators use "Yields")
• Raises: Document exceptions that callers should handle
• Yields: For generators, document yielded values

Class Docstrings

class SampleClass:
    """Summary of class here.

    Longer class information describing what the class represents
    (not that it "is a class").

    Attributes:
        likes_spam: A boolean indicating spam preference.
        eggs: An integer count of eggs we have.
    """

    def __init__(self, likes_spam: bool = False):
        """Initializes the instance.

        Args:
            likes_spam: Defines instance preference.
        """
        self.likes_spam = likes_spam
        self.eggs = 0

Comments

• Block comments: Full sentences, capitalized, period at end
• Inline comments: 2+ spaces from code, used sparingly
• Tricky code: Comment before the operation
• Non-obvious code: Comment at end of line
• TODO format: # TODO: bug-reference - Description or # TODO(username): Description
• Keep comments up-to-date with code changes!
• Comments in English unless 120% sure code never read by non-speakers

Override Methods

• Use @override decorator (from typing_extensions) when overriding
• No docstring needed if behavior unchanged
• Add docstring if behavior differs or side effects added

5. Type Hints (PEP 484 + Google)

Basic Rules

• Strongly encouraged for function signatures
• Use for complex functions, public APIs, when types aren't obvious
• Don't annotate self or cls (except when needed for proper type info - use Self)
• Don't annotate __init__ return (always None)

Type Hint Style

def my_method(
    self,
    first_var: int,
    second_var: Foo,
    third_var: Bar | None,
) -> int:
    ...

Modern Syntax (Python 3.10+)

• Use | for unions: str | None (not Optional[str] or Union[str, None])
• Use built-in types: list[int], dict[str, int] (not List[int], Dict[str, int])
• Use collections.abc for parameters: Sequence, Mapping (not concrete types)

Specific Guidelines

• Use explicit X | None not implicit (a: str = None is wrong)
• Specify generic parameters: list[int] not bare list
• Use Any when best type unknown (but prefer TypeVar when possible)
• Type aliases: CapWords naming, use TypeAlias annotation
• Forward references: use from __future__ import annotations or string quotes
• Conditional imports: use if TYPE_CHECKING: for type-only imports

Type Variable Naming

_T = TypeVar("_T")  # Good: leading underscore, descriptive
_P = ParamSpec("_P")  # Good: leading underscore
AddableType = TypeVar("AddableType", int, float, str)  # Good: descriptive

6. Code Quality & Best Practices

Implicit False (PEP 8)

# Good
if not seq:
if foo is None:
if not x:

# Bad
if len(seq) == 0:
if foo == None:
if x == False:

Comparisons

• Singletons: use is/is not: if x is None:
• Use is not rather than not ... is
• Don't compare booleans to True/False: if greeting: not if greeting == True:
• Type checking: use isinstance(obj, int) not type(obj) is int
• String prefixes/suffixes: use .startswith()/.endswith() not slicing

Sequences

• Use empty sequence truth value: if seq: not if len(seq):
• Works for strings, lists, tuples

Exception Handling

• Never use bare except: (catches SystemExit/KeyboardInterrupt!)
• Use specific exceptions: except ValueError: not except Exception:
• Minimize try block scope (avoid masking bugs)
• Use finally for cleanup or prefer context managers
• Exception chaining: raise X from Y or raise X from None
• Derive from Exception not BaseException
• Exception names end in Error (if they are errors)

String Formatting

# Good - Modern (preferred)
x = f'name: {name}; score: {n}'

# Good - Classic
x = 'name: %s; score: %d' % (name, n)
x = 'name: {}; score: {}'.format(name, n)

# Bad - Don't concatenate in loops
employee_table = '<table>'
for last, first in employees:
    employee_table += f'<tr><td>{last}, {first}</td></tr>'  # BAD!

# Good - Use list + join
items = ['<table>']
for last, first in employees:
    items.append(f'<tr><td>{last}, {first}</td></tr>')
employee_table = ''.join(items)

Logging

# Good - Use %-style (not f-strings!)
logger.info('TensorFlow version: %s', tf.__version__)

# Bad - Don't use f-strings (prevents lazy evaluation)
logger.info(f'TensorFlow version: {tf.__version__}')

Resource Management

# Good - Always use context managers
with open('file.txt') as f:
    data = f.read()

# Good - For non-context objects
import contextlib
with contextlib.closing(urllib.urlopen("http://...")) as page:
    for line in page:
        print(line)

# Bad - Don't rely on __del__ or manual close
f = open('file.txt')
data = f.read()
f.close()  # May not run if exception occurs!

Function Defaults

# Good - No mutable defaults
def foo(a, b=None):
    if b is None:
        b = []

# Good - Immutable default
def foo(a, b: Sequence = ()):
    ...

# Bad - Mutable default (shared across calls!)
def foo(a, b=[]):
    ...

Comprehensions

# Good
result = [mapping_expr for value in iterable if filter_expr]

# Bad - Multiple for clauses
result = [(x, y) for x in range(10) for y in range(5) if x * y > 10]

# If complex, use regular loop
result = []
for x in range(10):
    for y in range(5):
        if x * y > 10:
            result.append((x, y))

Lambdas & Operators

# OK for simple cases
sorted_list = sorted(items, key=lambda x: x.name)

# Better - Use operator module
from operator import attrgetter
sorted_list = sorted(items, key=attrgetter('name'))

# Bad - Multi-line lambda
complicated = lambda x: x.filter(something).map(
    another_thing).reduce(final_thing)

# Good - Use def
def complicated(x):
    return x.filter(something).map(another_thing).reduce(final_thing)

Statements

# Good
if foo == 'blah':
    do_blah_thing()
do_one()
do_two()

# Bad - Compound statements
if foo == 'blah': do_blah_thing()
do_one(); do_two(); do_three()

Return Statements

• Be consistent: all return expressions or all return None
• Explicit is better: use return None not bare return if other returns have values

Properties

• Use @property decorator (not manual descriptors)
• Only for trivial computations (cheap, straightforward)
• Don't use for expensive operations or complex logic
• Don't use just to wrap simple attribute access (make it public)

Decorators

• Use judiciously when clear advantage
• Never use @staticmethod (use module-level function - Google)
• Use @classmethod sparingly (named constructors, class-specific state)

Global State

• Avoid mutable global state
• Module-level constants OK: MAX_TIMEOUT = 30
• Name private globals with leading underscore: _internal_cache

Power Features (Avoid)

• No custom metaclasses
• No bytecode manipulation
• No __del__ for cleanup
• No reflection hacks (some getattr use is OK)
• No import hacks
• Standard library can use these (e.g., abc.ABCMeta, dataclasses, enum)

7. Security

SQL Injection

# Bad - SQL injection risk!
query = f"SELECT * FROM users WHERE id = {user_id}"
query = "SELECT * FROM users WHERE id = " + user_id

# Good - Use parameterized queries
query = "SELECT * FROM users WHERE id = %s"
cursor.execute(query, (user_id,))

Input Validation

• Validate all external input
• Use allowlists not denylists
• Sanitize before using in system commands

Hardcoded Secrets

• Never hardcode passwords, API keys, tokens
• Use environment variables or secret management
• Check for: password = "...", api_key = "...", etc.

Unsafe Functions

• Avoid: eval(), exec(), compile(), __import__()
• Be careful with: pickle, yaml.load() (use safe_load)

8. Performance

String Concatenation

• Never use + or += in loops (quadratic time!)
• Use ''.join(items) or io.StringIO

Generators

• Use generators for large sequences (memory efficient)
• Use comprehensions over map()/filter() with lambda

Default Iterators

# Good
for key in adict:
for line in afile:
for k, v in adict.items():

# Bad
for key in adict.keys():
for line in afile.readlines():

9. Maintainability

Function Length

• Prefer < 40 lines (Google guideline, not hard limit)
• Break up long functions unless it harms structure
• If >40 lines, consider if it can be split

Main Guard

# Good
def main():
    ...

if __name__ == '__main__':
    main()

# Or with absl
from absl import app

def main(argv):
    ...

if __name__ == '__main__':
    app.run(main)

Assertions

• Don't use assert for critical logic (can be disabled with -O)
• OK for validating test expectations
• Use if + raise for preconditions

Output Format

Structure your review as:

Summary

• Overall Assessment: Excellent/Good/Fair/Needs Improvement
• PEP 8 Compliance: High/Medium/Low
• Google Style Compliance: High/Medium/Low
• Key Strengths: 2-4 well-implemented aspects
• Critical Issues: Issues requiring immediate attention (if any)

Detailed Findings

Group by category. For each issue:

[Category: Style/Documentation/Quality/Security/Performance/Maintainability]

Issue #: Brief title

• Severity: Critical/High/Medium/Low
• Lines: Specific line numbers
• PEP 8/Google Reference: Section reference (if applicable)
• Description: Clear explanation of the issue
• Current Code:

  # Problematic code excerpt
  

• Recommended Fix:

  # Corrected code
  

• Rationale: Why this matters (readability/safety/performance/maintainability)

Positive Highlights

• Well-implemented patterns worth noting
• Good adherence to standards
• Exemplary practices

Recommendations

• Priority-ordered list of improvements
• Consider quick wins vs. larger refactors
• Balance consistency with practical constraints

References

• PEP 8 Style Guide
• Google Python Style Guide
• PEP 257 Docstring Conventions
• PEP 484 Type Hints

Enforcement Tools

Recommended:

• pylint: Comprehensive linter (Google's pylintrc)
• pytype: Type checker (Google's tool)
• mypy: Alternative type checker
• Black or Pyink: Auto-formatters (Google uses these)
• flake8: Alternative linter
• isort: Import sorting

Suppression:

• Use # pylint: disable=rule-name with explanation
• Use # type: ignore for type checking (sparingly)
• Document why suppression is needed

Review Guidelines

Be Constructive

• Explain why something matters
• Provide specific, actionable recommendations
• Include code examples for fixes
• Acknowledge good practices

Context Matters

• Consider project conventions
• Match surrounding code style when editing
• Balance improvement with backwards compatibility
• Know when rules have valid exceptions

Prioritize

1. Critical: Security issues, correctness bugs
2. High: Significant readability/maintainability issues
3. Medium: Style violations, minor best practices
4. Low: Nitpicks, suggestions

Pragmatic Approach

• Focus on changes being made (not rewriting entire codebase)
• Suggest incremental improvements
• Consider team capacity and priorities
• "Perfect is the enemy of good"

Special Cases

Legacy Code

• Focus on new/modified code
• Don't require full refactor to meet standards
• Suggest incremental modernization

Mathematical/Scientific Code

• Short variable names OK if match notation: i, j, x, y
• Reference paper/algorithm in comments
• Use # pylint: disable=invalid-name if needed

Test Files

• PEP 8 compliant names: test_<method>_<state>
• Or legacy style: testMethodUnderTest_state
• Less strict docstring requirements

Backwards Compatibility

• Don't break compatibility just to comply with PEP 8
• Consider deprecation path for API changes

---

This skill combines the authoritative standards of PEP 8 with Google's battle-tested practices to provide comprehensive, practical Python code reviews.