Conversation
Signed-off-by: Steve Coffman <steve@khanacademy.org>
Signed-off-by: Steve Coffman <steve@khanacademy.org>
|
@egonelbre This includes PR #1 but I can separate them if you would like |
There was a problem hiding this comment.
I need to consider what to include and what not to include from these.
The main goals for the builtin prompts are to:
- be useful for all projects
- focus on correctness issues (mainly)
- be good examples how to build your own
More prompts also means that things run slower and more expensive.
It would be an interesting idea to have a way to specify different categories, e.g. "Go Conventions" or "HTTP" or "Database" related. That way we could have some more stylistic prompts without encumbering the base experience.
PS: the base prompts have definitely not been finalized, they are merely first iteration of them. So, I definitely want to improve them.
|
|
||
| ## Functions (mutually recursive group) | ||
| {{range .Functions}} | ||
| ### {{.Name}} |
There was a problem hiding this comment.
One of the reasons I avoided including comments is, because my gut feeling is that it might make LLM think of the wrong thing. i.e. comment contains something that the code contradicts.
There might be a better way to handle this though. e.g. cross check against docs and the func content instead of just trusting godoc/body together.
|
|
||
| ## Context Patterns | ||
|
|
||
| - context.Context stored as a struct field — it must be passed as an explicit function parameter |
There was a problem hiding this comment.
This is not quite always the case. Sometimes contexts are used to control lifecycle for several goroutines. Alternatively, it could be passed inside a wrapper struct, which contains additional info.
| and-channel design would eliminate the shared state entirely | ||
| - errgroup or fan-in channel not used to collect results from parallel goroutines — instead, | ||
| results accumulated via a shared slice with a mutex, or goroutines write directly to an | ||
| external resource rather than sending immutable values to an owner |
There was a problem hiding this comment.
I can see all of these Design category things being necessary in some scenarios due to performance.
| - time.Now() compared against another time.Now() across goroutines to determine ordering — | ||
| wall clocks drift and NTP can move time backward; use a monotonic measurement (time.Since, | ||
| time.Now().Add) for elapsed time and timeouts; use wall-clock time only for display | ||
| - time.Now(), rand, or os.Getenv called inside concurrent logic instead of injected at the |
There was a problem hiding this comment.
These while not ideal, I'm not sure how well this would catch issues.
| @@ -0,0 +1,36 @@ | |||
| You are reviewing Go code for context and authorization pattern violations. | |||
There was a problem hiding this comment.
Context use issues and authorization seem very different categories to me and can be handled separately.
| ## Retries | ||
|
|
||
| - Retry loop uses time.Sleep with a fixed interval — use exponential backoff with random jitter | ||
| (base * 2^n + jitter) to avoid coordinated retry storms when many clients fail simultaneously |
There was a problem hiding this comment.
This would also need an upper limit. Or maybe in general "use best practice exponential backoff" without specifying exactly which one.
| ## CRUD Conventions | ||
|
|
||
| FindByID / FindOne: | ||
| - Returns (nil, nil) when the entity does not exist — must return ENOTFOUND |
|
|
||
| FindByID / FindOne: | ||
| - Returns (nil, nil) when the entity does not exist — must return ENOTFOUND | ||
| - Does not wrap the callee's error with an Op before returning |
There was a problem hiding this comment.
I think this was already handled somewhere else.
| - Does not wrap the callee's error with an Op before returning | ||
|
|
||
| FindMany / List: | ||
| - Result slice declared with var (encodes as JSON null) — initialize with make([]*T, 0) |
There was a problem hiding this comment.
Just make([]*T) would be fine, the 0 doesn't add anything.
|
|
||
| FindMany / List: | ||
| - Result slice declared with var (encodes as JSON null) — initialize with make([]*T, 0) | ||
| - Returns ENOTFOUND for an empty result set — an empty list is not an error |
There was a problem hiding this comment.
ENOTFOUND is more of a C style error name.
|
PS: I initially had more categories 05e6e9b, but decided to trim them. |
2 participants
Hello again, I've been adding some additional categories and more prompts to try to guide my dreamlint with practices