Add GitHub Copilot review instructions#2011
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
There was a problem hiding this comment.
Pull request overview
Adds a repository-specific .github/copilot-instructions.md file to steer GitHub Copilot’s automated PR reviews toward MetricFlow’s contribution gates and high-risk correctness areas, reducing maintainer triage load.
Changes:
- Introduces Copilot review guidance covering pre-merge gates (issue/CLA/changelog), PR scope expectations, and interface-change escalation.
- Adds checklists for semantic correctness, cross-engine SQL compatibility, snapshot-based testing expectations, and package boundary rules.
- Documents reviewer comment standards and out-of-scope guidance to reduce review noise.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| - Bug fixes must include a regression test that would have failed before the fix. | ||
| - New features must include unit tests and, for anything that touches SQL generation, at least one snapshot test demonstrating expected output. | ||
| - Tests for pure Python logic belong in `tests_metricflow_semantics/` or `tests_metricflow_semantic_interfaces/`. |
There was a problem hiding this comment.
Not quite the right description. Maybe just that tests for code that is defined in metricflow go into test_metricflow, etc.
| 1. **Correctness** — the metric logic must be right | ||
| 2. **Performance** — SQL should be efficient on the target compute platform | ||
| 3. **Legibility** — generated SQL should be readable by an analyst | ||
| 4. **Ease of manipulation** — the code that generates SQL should be maintainable |
There was a problem hiding this comment.
Oof, I should update TENENTS.md to move this up.
|
|
||
| Every PR that introduces a user-visible change (new feature, bug fix, behavior change, deprecation) must include a file in `.changes/unreleased/` generated by `changie new`. If this entry is absent and the change is not purely internal (test-only, docs-only, CI-only), comment: | ||
|
|
||
| > This PR appears to be missing a `changie` changelog entry. Run `changie new` from the repo root, follow the prompts, and commit the generated file in `.changes/unreleased/`. Do not edit `CHANGELOG.md` directly. See [CONTRIBUTING.md](../CONTRIBUTING.md#adding-or-modifying-a-changelog-entry) for details. |
There was a problem hiding this comment.
Does this handle the Skip Changelog tag?
|
|
||
| ### One PR, one concern | ||
|
|
||
| A PR should do one thing. This is the single most important property for receiving a fast, high-quality review. Mixed concerns — a bug fix bundled with a refactor, a new feature alongside unrelated cleanup — make it harder to reason about correctness and harder to revert individual changes if a regression is discovered. When a PR conflates independent concerns, suggest separation: |
There was a problem hiding this comment.
An exception for minor renames / cleanup in an separate commit would be helpful to allow.
Why
The MetricFlow maintainer team reviews a high volume of external contributions from contributors with varying experience levels. Configuring Copilot's automated review behavior with explicit, repo-specific instructions reduces the manual triage burden by catching common issues (missing issue links, CLA failures, missing changelogs, untested changes, interface changes without prior discussion) before a human reviewer is pulled in.
What
Adds
.github/copilot-instructions.mdwith guidance across the following areas:changie)metricflow_semantic_interfaces/, query API, CLI, dataflow plan nodes, SQL rendering contracts) with OSI multi-org contextRefs
Drafted by claude-sonnet-4-6 under the direction of @theyostalservice