Add basic assumptions to AGENTS.md#3552
Conversation
| - Formatting and linting are enforced by `ruff` via `prek` (pre-commit): line length **130**, double-quoted strings, isort with `pyiceberg`/`tests` as first-party. Run `make lint` — ruff autofixes most issues. | ||
| - Full type annotations are required (`mypy` runs in strict mode: `disallow_untyped_defs`, `no_implicit_optional`, `warn_unused_ignores`). Avoid Any types. | ||
| - Docstrings follow the project's pydocstyle config; one-line summaries on public functions. No personal pronouns in comments. | ||
| - Define typed exceptions in `pyiceberg/exceptions.py` and raise the most specific one; don't raise bare `Exception`. |
There was a problem hiding this comment.
nit: ValueError is widely used in this project. We could rephrase it like this:
Use domain-specific exceptions from pyiceberg/exceptions.py for Iceberg-specific error conditions. For invalid arguments/values, ValueError is acceptable. Don't raise bare Exception.
| ## Testing | ||
|
|
||
| - Bias towards adding tests to existing files, rather than creating new files. | ||
| - Use existing test fixtures when possible. |
There was a problem hiding this comment.
Should we add a line on our preference for integration testing over mocking?
|
|
||
| - Bias towards adding tests to existing files, rather than creating new files. | ||
| - Use existing test fixtures when possible. | ||
|
|
There was a problem hiding this comment.
Maybe we should also say, avoid binaries if possible (for example, we use fastavro a lot to generate test-files).
|
Thanks @rambleraptor for extending this, I like it a lot! |
Co-authored-by: Fokko Driesprong <fokko@apache.org>
jayceslesar
left a comment
jayceslesar
left a comment
There was a problem hiding this comment.
It might be good to add something about comparing to upstream implementations when possible (java)
|
It might be good to add something about looking for existing PRs as well.. it seems recently that there are clearly some automations on "easier" issues where 2-3 PRs pop up and it makes it difficult to pick one to review hahahahah, additionally this is good to build of work someone may have done previously and adhere to guidelines about preserving that work! |
4 participants
Rationale for this change
Our AGENTS.md just talks about the security model. We get a non-trivial amount of AI activity on this repo and our AGENTS.md should reflect some basic assumptions we want for those PRs.
A lot of this was inspired by Iceberg AGENTS.md.
This is just meant to be an initial version with some thoughts that I've seen and some of the basic architecture (as seen in the Iceberg AGENTS.md)
Are these changes tested?
Just Markdown.
Are there any user-facing changes?
No.