Add GitHub Copilot review instructions#3142
Add GitHub Copilot review instructions#3142aryanmehrotra wants to merge 8 commits intodevelopmentfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a GitHub Copilot instruction document to standardize code review expectations for GoFr contributions, with framework-specific guidance around API stability, error handling, linting, testing, and idiomatic Go practices.
Changes:
- Introduces a comprehensive GoFr-focused code review guideline document for Copilot.
- Documents repo context (module, Go versions, key directories) and key architectural principles (no globals/init, lean interfaces, context-first).
- Provides references for error handling, logging, linting, testing, and documentation expectations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…erage tooling, logging layer
|
The instructions look okay to begin with. However currently the rules are written keeping REST as primary focus. Need to be extended for other Modules also. Bu would like to have that iteratively. Hence this PR looks god to me. |
Umang01-hash
left a comment
There was a problem hiding this comment.
1. In section 1 we missed the Multi-module architecture in GoFr like our datasources having their own go.mod. Any reviewer who doesn't know this will:
- Miss dependency changes in datasource PRs
- Incorrectly assume a dependency change in one datasource affects the core module
- Miss when a datasource PR needs its own version bump
We should include Multi-module repo. Every pkg/gofr/datasource/*/ subdirectory that contains a go.mod is a separate Go module. Changes to a datasource's dependencies are scoped to that module only. PRs touching multiple modules need each module's go.sum updated independently.
|
|
||
| ## 1. Repository Context | ||
|
|
||
| - **Module:** `gofr.dev` | **Go:** 1.25+ (CI tests on 1.23, 1.24, 1.25) |
There was a problem hiding this comment.
It should be: Go: 1.25 (minimum in go.mod; CI compatibility-tests on 1.23, 1.24, 1.25)
The go.mod says go 1.25.0, so the minimum is 1.25. The CI matrix tests on older versions for compatibility verification.
| - Based only on the diff (reference surrounding code for context only) | ||
| - Framework-aware: every change affects all downstream users | ||
|
|
||
| **Required Feedback Structure:** |
There was a problem hiding this comment.
The 5-point feedback structure has API Impact, Correctness, Performance, Testing, Documentation — but no observability point. For a framework that markets "built-in observability" this is a glaring omission. Lets add:
Observability: Does the change include proper OTel spans? New datasource operations should have traces. New pub/sub backends must follow the tracing pattern. Metrics should
be registered for any new operation that has latency/error characteristics.
| - **PR Awareness:** Always read the PR title and description to understand intent and scope. | ||
| - **This is a framework, not a service.** Every change affects all downstream users. API stability, backward compatibility, and performance implications carry extra weight. | ||
| - **Key directories:** | ||
| - `pkg/gofr/` — Core framework (App, Context, Router) |
There was a problem hiding this comment.
These are missing from the key directories list:
pkg/gofr/datasource/pubsub/— PubSub backends (most active area)pkg/gofr/service/— HTTP service-to-service clients with tracingpkg/gofr/datasource/file/— File store abstractions
|
|
||
| ### Interfaces & Types | ||
| - **The bigger the interface, the weaker the abstraction.** `io.Reader` (1 method) is powerful. An interface with 20 methods is a concrete type in disguise. | ||
| - **Accept interfaces, return structs.** Functions should take the narrowest interface they need and return concrete types. This maximizes flexibility for callers. |
There was a problem hiding this comment.
Correct Go idiom and GoFr's own convention is "return concrete types." A concrete type includes a pointer to a struct (*MyType), a slice, a map. "Return structs" is technically narrower and wrong in many cases. Change to "return concrete types."
|
|
||
| - **Never break existing public API** without deprecation path. GoFr follows semver. | ||
| - Exported functions MUST have godoc comments. `revive:exported` enforces this. | ||
| - New config keys must be documented and follow existing naming conventions (uppercase underscore). |
There was a problem hiding this comment.
Can we add some examples here like : PUBSUB_BACKEND, TRACE_EXPORTER, HTTP_PORT
Summary
Add comprehensive GitHub Copilot code review instructions tailored to the GoFr framework.
What's Included
Framework-Specific Context
Core Principles (from CONTRIBUTING.md)
gochecknoglobals/gochecknoinitsenforcedos.Getenv(), no singletonsError Handling Reference
ErrorEntityNotFound(404/INFO),ErrorInvalidParam(400/INFO),ErrorServiceUnavailable(503/ERROR), etc.StatusCode()+LogLevel(), wrap with context (err113), don't double-logGo Proverbs & Idiomatic Principles
Linting Requirements
.golangci.yml: 45+ linters, funlen (100/50), gocyclo (10), lll (140), American English//nolintmust specify linter + explanationTesting, API Compatibility, Documentation