-
Notifications
You must be signed in to change notification settings - Fork 62
Add google style guide docstrings to AGENTS.md #747
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,6 +40,57 @@ Use the `dev-container` skill for build, start, attach, and exec inside the cont | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - Prefer `assert condition, "message"` over `if not condition: raise ValueError("message")` for internal invariant checks. (Formatting, imports, and typing are enforced by `pre-commit` — see `.pre-commit-config.yaml`.) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - PR bodies follow `.github/pull_request_template.md` — a one-line Summary plus 2–5 detail bullets. Resist the agent default of long, multi-section descriptions. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ## Coding Style | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
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. We already have a coding style section just above. Suggestion to remove. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ### Checks/Asserts | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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.
Suggested change
Also: "(i.e)" on the next line should be "(i.e.,)". |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| In Arena we prefer asserts over if...raises in most cases where such an check represents a coding error | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
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. Looks like a duplicate of what we aleady state above on line 40, eg |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (i.e) something that shouldn't be recovered from. Asserts are briefer than the exception based counterpart. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+47
to
+48
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.
Suggested change
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| The raises-based approach is more verbose (for example it always uses multiple lines), and because | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this type of error is not intended to be recovered from, it offers no advantage. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ### Docstrings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
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. I feel like the follwing is a pretty verbose paragraph to describe how to write (good) docstrings. Slightly ironic given the existing doc explicitly states " Did you have any luck in adding a concise section to the coding style section like maybe with a short example?
Collaborator
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. Eg we refer to the google styleguide (Edit: maybe without linking the URL) and then only state our deltas explicitely |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Prefer short docstrings. Where a single line is not sufficient, a short (i.e. 2-3 line) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| paragraph can follow. Document args and returns, but not raises. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| We follow google-style for docstrings which is repeated below: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
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. google-style -- URL? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| A docstring should give enough information to write a call to the function without reading the function’s code. The docstring should describe the function’s calling syntax and its semantics, but generally not its implementation details, unless those details are relevant to how the function is to be used. For example, a function that mutates one of its arguments as a side effect should note that in its docstring. Otherwise, subtle but important details of a function’s implementation that are not relevant to the caller are better expressed as comments alongside the code than within the function’s docstring. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #### Args | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| List each parameter by name. A description should follow the name, and be separated by a colon followed by either a space or newline. If the description is too long to fit on a single 80-character line, use a hanging indent of 2 or 4 spaces more than the parameter name (be consistent with the rest of the docstrings in the file). The description should include required type(s) if the code does not contain a corresponding type annotation. If a function accepts *foo (variable length argument lists) and/or **bar (arbitrary keyword arguments), they should be listed as *foo and **bar. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #### Returns: (or Yields: for generators) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Describe the semantics of the return value, including any type information that the type annotation does not provide. If the function only returns None, this section is not required. It may also be omitted if the docstring starts with “Return”, “Returns”, “Yield”, or “Yields” (e.g. """Returns row from Bigtable as a tuple of strings.""") and the opening sentence is sufficient to describe the return value. Do not imitate older ‘NumPy style’ (example), which frequently documented a tuple return value as if it were multiple return values with individual names (never mentioning the tuple). Instead, describe such a return value as: “Returns: A tuple (mat_a, mat_b), where mat_a is …, and …”. The auxiliary names in the docstring need not necessarily correspond to any internal names used in the function body (as those are not part of the API). If the function uses yield (is a generator), the Yields: section should document the object returned by next(), instead of the generator object itself that the call evaluates to. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #### Raises | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| We don't document raises in Arena | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #### Example | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Here is an example docstring | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ```python | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def fetch_smalltable_rows( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| table_handle: smalltable.Table, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| keys: Sequence[bytes | str], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require_all_keys: bool = False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. This is Google's internal example ( |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Mapping[bytes, tuple[str, ...]]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Fetches rows from a Smalltable. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Retrieves rows pertaining to the given keys from the Table instance | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| represented by table_handle. String keys will be UTF-8 encoded. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| table_handle: An open smalltable.Table instance. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| keys: A sequence of strings representing the key of each table | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| row to fetch. String keys will be UTF-8 encoded. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require_all_keys: If True only rows with values set for all keys will be | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| returned. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. The docstring example should have a blank line between the summary and the extended description per Google style: """Fetches rows from a Smalltable.
Retrieves rows pertaining to the given keys from the Table instance
...Without it, the example contradicts the guidance it's illustrating. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| A dict mapping keys to the corresponding table row data | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fetched. Each row is represented as a tuple of strings. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+78
to
+90
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.
Suggested change
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ## Conventions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ### Wrapped Environment | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
This creates a duplicate top-level section — the file already has
## Coding style(lowercase 's') at line 38 with assert guidance. Recommend merging the new content into the existing section rather than creating a parallel one.(and remove the old
## Coding stylesection above, consolidating its content here)