Allow suppressing Python no-client warnings - #11823
Conversation
microsoft-github-policy-service
Bot
added
the
emitter:client:python
commit: |
|
All changed packages have been documented.
Show changes
|
There was a problem hiding this comment.
🟡 Changes recommended
The new diagnostic target selection can throw when no @service exists, and the commented-out early return allows generation to proceed despite having no clients.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adjusts the Python HTTP client emitter’s no-sdk-clients diagnostic so it can be suppressed via #suppress, and adds a regression test plus a Chronus change entry for the behavior.
Changes:
- Downgrade
no-sdk-clientsfrom error to warning in the emitter library diagnostics. - Report
no-sdk-clientson the service namespace (instead ofNoTarget) to enable#suppressto apply. - Add a regression test validating the diagnostic target and suppression behavior, and add a Chronus changelog entry.
File summaries
| File | Description |
|---|---|
| packages/http-client-python/emitter/test/emitter.test.ts | Adds a regression test ensuring no-sdk-clients targets the service namespace and is suppressible. |
| packages/http-client-python/emitter/src/lib.ts | Changes no-sdk-clients diagnostic severity from error to warning. |
| packages/http-client-python/emitter/src/emitter.ts | Introduces a helper to choose a suppressible diagnostic target and updates no-sdk-clients reporting logic. |
| .chronus/changes/fix-python-no-sdk-clients-suppression-2026-09-02.md | Adds a Chronus entry documenting the suppression behavior change. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Python emitter diffBaseline No changes to generated output. Rendered diff: inline on the run summary, or the emitter-diff-html artifact. Informational check (eng/emitter-diff); does not block the PR. |
|
You can try these changes here
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, aligns with the stated issue/requirements, and includes a focused regression test covering the new suppression target behavior.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new test host likely doesn’t mount the emitter package into the virtual test filesystem (libraries list is empty), making the new tests brittle and potentially failing with emitter resolution errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9baae876-cdbf-4556-952a-e95b26298ca9
There was a problem hiding this comment.
🟢 Approval recommended
The changes align with the stated issue/requirements and include focused regression tests validating suppression targeting and model-only generation behavior.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Drive the model-only test off the emitter's code model output (emit-yaml-only) rather than booting the Python/Pyodide generator, so it runs in ~400ms without a long timeout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9baae876-cdbf-4556-952a-e95b26298ca9
There was a problem hiding this comment.
🟡 Changes recommended
The no-sdk-clients fallback diagnostic target can still be effectively unsuppressible in common no-service programs (and the new test helper leaves behind OS temp YAML files), so suppression and test hygiene should be tightened before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
Copilot code review runs on every PR here with no knowledge of the rules this repo actually enforces. It doesn't know that `no-sdk-clients` reported on `NoTarget` can never be suppressed, that a `feat` changeKind is invalid, that appending a `SyntaxKind` member is fine but inserting one is not, or that each package exports a shared `Tester` from `test/test-host.ts`. So it reviews TypeSpec PRs like generic TypeScript. This adds `.github/skills/code-review/SKILL.md`, which Copilot code review picks up automatically from `.github/skills`. It gives the reviewer five things to check, each pointing at the file that defines the rule: - **Breaking changes** — the Tier 0–3 table from `breaking-change-policy.mdx`, plus the bug exception. An undeclared breaking change is the top finding. - **Changesets** — the six valid `changeKind` values from `.chronus/config.yaml`, per-package/per-type splitting, and the `changedFiles` exclusions so docs-only PRs aren't nagged. - **Compiler API usage** — diagnostics targeted at `NoTarget` are never suppressible and `error` severity can't be suppressed at all ([`program.ts`](https://github.com/microsoft/typespec/blob/main/packages/compiler/src/core/program.ts)); `listServices(program)[0]` is `undefined` for model-only programs; a severity downgrade changes control flow via `program.hasError()`. - **Tests** — reuse the package's shared tester instead of `createTester` per file, assert with `expectDiagnostics`/`expectDiagnosticEmpty` rather than hand-rolled `diagnostics.some(...)`, and drive the real entrypoint. - **Emitter output stability** — unexplained snapshot diffs, inline-vs-hoist, `$ref` correctness. It also says what *not* to comment on: formatting, lint-covered rules, style preferences, generated artifacts. ### Does it work? Dry-run against microsoft#11823. The skill surfaces the three points reviewers raised by hand there: | Skill rule | Human comment it reproduces | |---|---| | §3 `NoTarget` is not suppressible | "if there is no service this will still not be suppressable" | | §4 shared tester | "you shouldn't be creating a tester for every file" | | §4 `expectDiagnostics` | "reason can't use expectDiagnostics?" | It additionally flags the new test importing `getNoSdkClientsDiagnosticTarget`, which isn't exported from `src/emitter.ts`, and the diagnostic message that still says the generator "expects at least one client" after it was downgraded to a warning. An earlier draft excluded `packages/http-client-*`; that exclusion hid every finding above, so the skill now covers those packages too and defers to `.github/instructions/*.instructions.md` for their specifics. No changeset: `.github/**` isn't a published package.
Point the no-sdk-clients diagnostic at the first SDK model's raw type when there is no service, so it stays suppressable for model-only packages whose models live under an explicit namespace (the global namespace has no suppressable node). Also delete the code-model YAML temp file the emit-yaml-only test helper produces. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9baae876-cdbf-4556-952a-e95b26298ca9
There was a problem hiding this comment.
🔵 Needs a closer look
The new emitter test host’s virtual package should re-export $lib (not just $onEmit) to match real emitter loading behavior and ensure option/diagnostic metadata is exercised in tests.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/http-client-python/emitter/test/test-host.ts:30
- The virtual
@typespec/http-client-pythonpackage used for.emit()only re-exports$onEmit. In real usage the emitter entrypoint also exports$lib, which enables emitter option validation and registers the library’s diagnostics metadata during loading. Without re-exporting$libhere, these tests can diverge from real runtime behavior (e.g. invalid emitter options won’t be caught).
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes align with the stated issue/acceptance criteria, include focused regression coverage, and appear low-risk and internally consistent.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
| // linked into its own `node_modules` and `.emit()` can't resolve it by name. Register a minimal | ||
| // virtual package whose entrypoint re-exports the real `$onEmit`, so the emitter under test runs. | ||
| export const EmitterTester = PythonTester.files({ | ||
| "node_modules/@typespec/http-client-python/package.json": JSON.stringify({ |
There was a problem hiding this comment.
from talking with iscai-msft offline I thikn we want to hold off on using this in js unit test if it calls the python exec, this makes the test extermely slow. I think we should be able to test most funcitons in the emitter with the PythonTester defined above and just call the functions directly/mock the call to the python process
There was a problem hiding this comment.
we won't be calling the python exec, this is bc we need to have the tester framework access to our package since we're not in the monorepo. we're going to only call the emitter step which is all typescript, not python exec call
For #11794
Summary
no-sdk-clientsfrom an error to a warning#suppresscan consume it@typespec/http-client-pythonTesting
pnpm vitest run emitter/test/emitter.test.ts --config emitter/vitest.config.ts(1 test passed)