Document Python import style guidelines in AGENTS.md - #299
Document Python import style guidelines in AGENTS.md#299Bernhard Merkle (bmerkle) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Documents the project’s preferred Python import style in AGENTS.md as groundwork for future import-consistency cleanup work, but also includes a few unrelated dependency/code formatting updates.
Changes:
- Add a “Python Import Style Guidelines” section to
AGENTS.md(module-qualified by default; limit direct-symbol imports; forbid wildcard imports). - Add a new date/time range guideline (half-open intervals) to
AGENTS.md. - Bump
pyright(and related lockfile entries) and widen theuv_buildupper bound; reorder one stdlib import inanswers.py.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| AGENTS.md | Adds documented Python import-style rules (and an additional date/time range guideline). |
| pyproject.toml | Updates build-system constraint and dev dependency minimum for pyright. |
| uv.lock | Updates the locked pyright version/metadata and dev specifier to match pyproject.toml. |
| src/typeagent/knowpro/answers.py | Adjusts stdlib import ordering at the top of the module. |
|
robgruen would you mind taking a look at this one when you get a chance? Thanks! |
Bernhard Merkle (bmerkle)
changed the title
Codifies the module-qualified-by-default convention discussed in issue microsoft#112, to guide the follow-up import-consistency cleanup.
850b9ea to
ab186e2
Compare
| ## 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`). | ||
| * **Prohibit Wildcard Imports:** Never use wildcard imports (`from module import *`) under any circumstances. | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
actually I just remembered that I had the same conversation with guido at some point and he didn't change his mind either.
There was a problem hiding this comment.
LGTM with that in mind
There was a problem hiding this comment.
Went ahead and added this: ruff is now a dev dependency with a make ruff target (63c62d8), scoped to F403/F405 (wildcard-import ban) since that's the one piece of this guideline that's genuinely mechanical to enforce. interfaces.py's sanctioned re-export aggregator is carved out via a per-file-ignore, and I tightened the AGENTS.md wildcard-import bullet to explicitly name that exception so the doc and the linter agree.
Not wired into make all/CI yet — following the same staged approach Guido van Rossum (@gvanrossum) suggested back in #116 ("make a small PR that allows us to run make ruff, adding it to CI is a separate step"). Let me know if you'd like CI wiring folded into this PR too, or tracked as the immediate next step.
There was a problem hiding this comment.
Kevin Turcios (@KRRT7) I have added first ruff integration, so we can run it locally and proceed incrementally.
Thanks for bringing up the idea again :-)
Bernhard Merkle (bmerkle)
changed the title
Deterministically enforces the "no wildcard imports" rule from the new Python Import Style Guidelines via ruff's F403/F405 checks, addressing KRRT7's request that this be pre-commit/CI-linted rather than prose-only. Scoped narrowly (not a full isort/black replacement, see microsoft#116): only F403/F405 are selected, and interfaces.py keeps its sanctioned re-export aggregator pattern via a per-file-ignore. `make ruff` is a standalone target for now, not wired into `all`/CI (staged per gvanrossum's suggestion in microsoft#116; CI wiring is a separate follow-up step). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
robgruen can you please review this PR ? Thanks a lot :-) |
Summary
interfaces.py).ruffas a dev dependency and amake rufftarget, scoped narrowly to deterministically enforcing the wildcard-import ban (F403/F405) via[tool.ruff.lint], addressing Kevin Turcios (@KRRT7)'s review request that this be pre-commit/CI-linted rather than prose-only.interfaces.py's aggregator pattern is carved out via[tool.ruff.lint.per-file-ignores]. Not wired intomake all/CI yet — staged per the incremental approach from consider ruff as alternative to black and isort etc #116, where broader ruff adoption was discussed and deferred; CI wiring is a follow-up.Rebased onto latest
mainto resolve merge conflicts — the date/time range guideline, pyright bump, andanswers.pyimport reorder that were previously part of this branch's diff are already onmainvia #297, so they dropped out as duplicates during the rebase.Test plan
make ruffpasses cleanly (verified locally).make format(isort + black) anduv run isort --check-onlyunaffected.