-
Notifications
You must be signed in to change notification settings - Fork 75
Document Python import style guidelines in AGENTS.md #299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ab186e2
63c62d8
c9d9728
0ebccbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,6 +120,16 @@ please follow these guidelines: | |
| - **Exception**: Explicit re-export patterns like `from ... import X as X` or marked with "# For export" | ||
| - This prevents circular imports and makes dependencies clear | ||
|
|
||
| ## Python Import Style Guidelines | ||
|
|
||
| * **Default to Module-Qualified Imports:** Prefer importing whole modules and using qualified calls (e.g., `import math; math.sqrt(16)` or `import pandas as pd; pd.DataFrame()`) to prevent namespace pollution, avoid name clashes, and provide immediate context for where functions or objects originate. | ||
| * **Use Direct Symbol Imports Cautiously:** Restrict direct imports (`from module import symbol`) to specific scenarios where they genuinely improve readability or adhere to standard conventions: | ||
| * Importing classes, exceptions, or constants (e.g., `from my_project.models import User`). | ||
| * Avoiding severe, repetitive visual clutter in heavy mathematical or algorithmic code. | ||
| * Standard library patterns (e.g., `from collections import defaultdict, Counter`). | ||
| * **Exception**: In `tests/`, importing functions directly to call them in assertions is idiomatic pytest style and is not held to this restriction (see #298). | ||
| * **Prohibit Wildcard Imports:** Never use wildcard imports (`from module import *`) in normal code. **Exception**: the same explicit re-export aggregator pattern allowed by the Import Architecture Rules above (e.g. `interfaces.py` re-exporting `interfaces_core`, `interfaces_indexes`, etc. with `__all__`) — enforced by `make ruff` (see Makefile), with that file listed in `[tool.ruff.lint.per-file-ignores]`. | ||
|
|
||
|
Comment on lines
+123
to
+132
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe ruff has some rules that enforce this, or at the very least, can be enabled, these sort of things should be deterministic and triggered by pre-commit / CI linting, it's context engineering best practices.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point, and for context this isn't new ground — I raised almost this exact question in #116 ("consider ruff as alternative to black and isort etc"). Guido van Rossum (@gvanrossum) and I went back and forth on it there (Ruff's isort-compatible config, mixed import/from ordering, etc.), and I closed it once #132 landed a working isort profile that covers the ordering piece we needed at the time: "via #132 we have now a working isort profile in place, so IMO we do not need to consider ruff further, at least for now." That said, your comment is really about a narrower and separate gap: isort only sorts/groups imports, it doesn't ban wildcard imports or enforce the qualified-vs-direct-import heuristic documented here. Neither of those was in scope of the #116 discussion. The wildcard-ban part is genuinely a one-line, zero-config win with ruff (F403/F405 are in its default rule set) — the qualified-vs-direct heuristic isn't something a standard rule enforces automatically (it needs to distinguish "is this a class/exception/constant", which isn't purely mechanical).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually I just remembered that I had the same conversation with guido at some point and he didn't change his mind either.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM with that in mind
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Went ahead and added this: Not wired into
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kevin Turcios (@KRRT7) I have added first ruff integration, so we can run it locally and proceed incrementally. |
||
| * Order imports alphabetically after lowercasing; group them as follows | ||
| (with a blank line between groups): | ||
| 1. standard library imports | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.