Document Python import style guidelines in AGENTS.md - #299
Bernhard Merkle (bmerkle) wants to merge 4 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 :-) |
|
robgruen can you please review this PR ? |
|
Apologies, totally missed this!!! |
Addresses robgruen's review nit on PR microsoft#299: the direct-symbol-import restriction doesn't apply to tests/, where importing functions directly for assertions is idiomatic pytest style per microsoft#298's scope decision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks robgruen!
|
|
## Summary - `knowledge_schema` exports classes only (`Quantity`, `Quantifier`, `Facet`, `ConcreteEntity`, `ActionParam`, `Action`, `KnowledgeResponse`), so per the import style guidelines in #299/AGENTS.md this is a direct-symbol-import case. - Removes the `from . import knowledge_schema as kplib` alias (flagged as a smell in [#112](#112 (comment))) everywhere it appears — 10 files in `src/typeagent`, 8 test files, 2 tool scripts — replacing `kplib.X` with a direct `from .knowledge_schema import X, Y, Z` import of the symbols actually used at each call site. - Mechanical change only; no behavior change. - Phase 1 of the import-consistency cleanup tracked in #298. ## Test plan - [x] `make format check test` — 0 pyright errors (Python 3.12 and 3.14), 737 passed / 12 skipped
## Summary - `reltermsindex` (memory variant) exports a mix of classes (`TermToRelatedTermsMap`, `RelatedTermsIndex`, `TermEmbeddingIndex`) and functions (`build_related_terms_index`, `resolve_related_terms`, `dedupe_related_terms`), so per the import style guidelines in #299/AGENTS.md it defaults to module-qualified. - Converts the 3 `src/typeagent` call sites (`knowpro/secindex.py`, `knowpro/search.py`, `storage/memory/provider.py`) from direct-symbol imports to `from ..storage.memory import reltermsindex` + qualified calls. - Bonus: in `search.py`, `resolve_related_terms` was both a free function (imported) and a method on `QueryCompiler` — qualifying the import removes that latent name shadowing. - The sqlite variant (`storage/sqlite/reltermsindex.py`) exports single classes each and is left as direct-symbol import — already compliant, no change. - Scope: `src/typeagent` only, per #298 — test files keep direct-symbol imports, which is idiomatic pytest style. - Phase 2 (1/3) of the import-consistency cleanup tracked in #298. ## Test plan - [x] `make` (format, check on 3.12/3.14, test, build) — 0 pyright errors, 737 passed / 12 skipped, wheel builds
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.