-
Notifications
You must be signed in to change notification settings - Fork 76
RFC: Add an AGENTS.md in c/image #636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| # Commitments | ||
|
|
||
| go.podman.io/image/v5 (aka c/image) promises to keep a stable Go API, | ||
| following the general Go semantic versioning rules, and not bumping | ||
| the major version. Keep that promise. | ||
|
|
||
| # Prioritize human attention | ||
|
|
||
| Avoid repetitive code. As a rule of thumb, 3 repetitions of | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of this looks good, however...I think some of it is better broken out into a separate STYLE.md - where it's also applicable for humans too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improving all of that would be nice, but it’s a “someday-maybe” task, while changing what agents do for my work, and for work that I have to review (e.g. the above is very frequently violated in AI-written tests), is comparatively much more urgent. |
||
| the same >5-line pattern should probably be refactored, _if_ | ||
| a clear abstraction can be found. | ||
|
|
||
| Human’s screens (and attention spans) are limited. Avoid very long | ||
| linear functions, look for ways to abstract / split the function | ||
| _if_ the resulting smaller functions have a clear purpose and interface. | ||
| Use blank lines within function bodies sparingly, less than | ||
| you would do by default (but do use them when separating large conceptually | ||
| different parts of the function’s code). | ||
|
|
||
| Don't add redundant comments that add no value. Code in style | ||
| // Add a user | ||
| ….addUser(…) | ||
| is _never_ acceptable. | ||
|
|
||
| # Tests | ||
|
|
||
| The default pattern is TestFunctionName, or TestObjectMethodName | ||
| (in that case, with no underscore), containing all tests for a function/method. | ||
| Do not _default_ to adding a new test function when adding a feature | ||
| to an existing function. | ||
|
|
||
| Tests should typically be table-driven. When choosing between a | ||
| semantically precise table types and short table entries, prefer short table entries | ||
| so that the whole test table easily fits on a screen. For example, usually don't | ||
| add .name fields to test tables - have such descriptions as comments on the same line | ||
| as the other test content. | ||
|
|
||
| It should be very rare to test error message text. Just a test that an error is reported | ||
| is frequently enough. | ||
|
|
||
| # Existing code | ||
|
|
||
| Have some (but not slavish) deference to existing code structure: don't | ||
| refactor a whole file just to add 3 lines, if that addition would be otherwise clean. | ||
|
|
||
| If some refactoring _is_ beneficial, the goal is to have one or more _pure_ | ||
| refactoring commits (which don't change the observable behavior at all, and document that | ||
| in the commit message), followed by a separate commit that adds the required feature. | ||
|
|
||
| # Documentation | ||
|
|
||
| Most data structures with scope larger than a single function probably need documentation. | ||
| Document field interactions: | ||
| UseTLS bool | ||
| TLSConfig … // Only used if UseTLS | ||
| special values: | ||
| Name // "" if unknown | ||
| _Never_ add comments that add no value: | ||
| // A user | ||
| type User struct { …} | ||
| or | ||
| Name // name | ||
| is never acceptable. | ||
|
|
||
| Most functions should have documentation, documenting in enough detail that, when working | ||
| on a caller code, humans can read only the callee’s documentation without reading the callee | ||
| itself — but no more! The documentation of a function should almost never contain _how_ | ||
| the function does it, the caller shouldn’t need to care. | ||
|
|
||
| If such a function documentation looks too convoluted, that’s a sign that the function’s interface | ||
| is probably not right — the function boundary is in the wrong place, or an abstraction is missing | ||
| to simplify the concepts. | ||
|
|
||
| Comments within function bodies should typically only document non-obvious implementation | ||
| decisions, non-obvious constraints that require using one approach over another, or _sometimes_ | ||
| delineate significantly conceptually separate parts of the same function (definitely not | ||
| after every blank line). It should not generally be necessary to document, in a caller, what calling another | ||
| function does — if that is confusing, the callee should probably be renamed or refactored. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it actually work to have AGENTS.md in a subdirectory like this? I don't think tools will read it unless invoked from that subdirectory which is likely unusual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least https://cursor.com/docs/context/rules#agentsmd suggests that works. I don‘t know how to tell for sure. (Asking the agent without any task reveals that it found the
image/AGENTS.mdfile.)Anecdotally, I have seen agent edits that suggest that these instructions were followed in content outside the
imagedirectory; I’m not sure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I see, I was unaware of this. I spent some tokens on this:
I wasn't aware of this. That said...I guess you're also trying to say that you work on only the image/ subdirectory and aren't trying to imply style/coding constraints for the rest of the repository...which I can understand, but OTOH seems to conflict with the monorepo goal?
None of this seems controversial and so it'd make sense to move it up a level anyways right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For historical reasons I feel sort of authoritative for coding style in
image. It’s weaker forstoragewhere I don’t have the history. And I have historically done very little incommonso I want to be much more cautious about imposing my aesthetic choices there. (E.g.libartifactis, I understand, intentionally using more verbose comments.)That said, it’s an RFC. If others like it, I’m not opposed to making this top-level.