Add google style guide docstrings to AGENTS.md by alexmillane · Pull Request #747 · isaac-sim/IsaacLab-Arena

alexmillane · 2026-06-01T08:01:59Z

Summary

Add docstring guidelines to AGENTS.md

Detailed description

Copies in guidelines from Google style (modified slightly to match our style).

greptile-apps · 2026-06-01T08:03:52Z

Greptile Summary

This PR adds a "Coding Style" section to AGENTS.md covering assert/raises preferences and Google-style docstring guidelines for AI agents to follow. The content itself is useful, but there are structural and formatting issues that should be addressed before merging.

A duplicate section header is introduced: ## Coding Style (new) sits immediately after the existing ## Coding style (line 38), with overlapping assert/raises guidance appearing in both — the new content should be merged into the existing section.
The example docstring is missing the blank lines between the summary, extended description, and Args:/Returns: sections that Google style mandates, meaning AI agents following the example would produce non-conforming docstrings.
Two minor grammar issues in the new prose: "such an check" and a malformed "(i.e)" abbreviation.

Confidence Score: 3/5

The PR is documentation-only, but the example docstring directly contradicts the Google style it claims to demonstrate, and the file now has two overlapping coding style sections — both issues would actively mislead agents following this guide.

The duplicate section and the incorrect example docstring (missing required blank lines between sections) are functional errors in a style guide document whose primary purpose is to instruct AI agents on correct formatting. An agent that faithfully copies the example will produce docstrings that don't match Google style.

AGENTS.md — the new section structure and the example docstring both need corrections.

Important Files Changed

Filename	Overview
AGENTS.md	Adds "Coding Style" section with Checks/Asserts and Docstrings subsections; introduces a duplicate section header alongside the existing "Coding style" section, the example docstring is missing required blank lines between Google-style sections, and there are two grammar issues in the new prose.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[AGENTS.md] --> B["## Coding style (existing, line 38)"]
    A --> C["## Coding Style (new, line 43)"]
    B --> B1["assert over if-raise bullet"]
    B --> B2["PR body format bullet"]
    C --> C1["### Checks/Asserts\n(duplicates B1 content)"]
    C --> C2["### Docstrings\nGoogle style guidelines"]
    C2 --> D["#### Args"]
    C2 --> E["#### Returns"]
    C2 --> F["#### Raises\n(says: not documented)"]
    C2 --> G["#### Example\n⚠️ Missing blank lines\nbetween sections"]

Comments Outside Diff (1)

AGENTS.md, line 38-50 (link)

Duplicate section with redundant assert/raises guidance

The file now has two separate coding style sections: ## Coding style (line 38) and the newly added ## Coding Style (line 43). The assert/raises preference is documented in both — as a bullet in the original section and again as the "Checks/Asserts" subsection. AI agents (and humans) parsing this file will encounter conflicting or duplicated guidance. The new content should be merged into the existing ## Coding style section rather than creating a second one.

_{Reviews (1): Last reviewed commit: "Add google style guide docstrings to AGE..." | Re-trigger Greptile}

greptile-apps · 2026-06-01T08:03:57Z

+    """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:
+        A dict mapping keys to the corresponding table row data
+        fetched. Each row is represented as a tuple of strings.
+    """


The example docstring is missing the blank lines between the summary sentence, the extended description paragraph, and the Args: section that Google style requires. As written, an AI agent following this example would produce non-conforming docstrings.

Suggested change

"""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:

A dict mapping keys to the corresponding table row data

fetched. Each row is represented as a tuple of strings.

"""

"""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:

A dict mapping keys to the corresponding table row data

fetched. Each row is represented as a tuple of strings.

""

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!

greptile-apps · 2026-06-01T08:03:58Z

+In Arena we prefer asserts over if...raises in most cases where such an check represents a coding error
+(i.e) something that shouldn't be recovered from. Asserts are briefer than the exception based counterpart.


Two minor grammar issues: "such an check" should be "such a check", and "(i.e)" is missing its comma and a closing parenthesis around the clause — the standard abbreviation is "(i.e.,)".

Suggested change

In Arena we prefer asserts over if...raises in most cases where such an check represents a coding error

(i.e) something that shouldn't be recovered from. Asserts are briefer than the exception based counterpart.

In Arena we prefer asserts over if...raises in most cases where such a check represents a coding error

(i.e., something that shouldn't be recovered from). Asserts are briefer than the exception based counterpart.

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!

isaaclab-review-bot

Review: Add google style guide docstrings to AGENTS.md

Good initiative to codify docstring conventions — this will help both human contributors and AI agents maintain consistency. A few issues to address before merging:

Key Findings

1. Duplicate section header (high)

The file already contains a ## Coding style section (line 38, lowercase 's') with the assert preference bullet. This PR adds a new ## Coding Style (uppercase 'S') section immediately after it. This creates two nearly-identically-named top-level sections, which is confusing for both readers and AI agents navigating by heading. The new content should either be merged into the existing section or the existing section should be replaced.

2. Redundant assert guidance (medium)

The existing ## Coding style section already states:

Prefer assert condition, "message" over if not condition: raise ValueError("message") for internal invariant checks.

The new ### Checks/Asserts sub-section restates this at greater length. Consider consolidating — either expand the existing bullet in-place and remove the new sub-section, or remove the existing bullet and keep only the new sub-section under a unified heading.

3. Grammar/punctuation nits (low)

"such an check" → "such a check"
"(i.e)" → "(i.e.,)" or "i.e.,"
"We don't document raises in Arena" — missing period
"Here is an example docstring" — missing period/colon

4. Example not project-relevant (suggestion)

The fetch_smalltable_rows example is copied directly from Google's style guide and references smalltable.Table, which has no connection to Isaac Lab/Arena. Consider replacing it with a small Arena-relevant example (e.g., a function from isaaclab_arena) to make the guidance more concrete for contributors working in this codebase.

5. Docstring example doesn't follow its own rules (medium)

Per Google style, a multi-line docstring should have a blank line between the summary line and the elaboration paragraph. The example shows:

"""Fetches rows from a Smalltable.
Retrieves rows pertaining to...

It should be:

"""Fetches rows from a Smalltable.

Retrieves rows pertaining to...

6. Trailing double blank line (nit)

There are two consecutive blank lines before ## Conventions — should be one.

Summary: The main structural issue is the duplicate heading and redundant content (findings 1 & 2). Once those are resolved and the minor grammar fixes applied, this looks good to merge. The example swap (finding 4) is a nice-to-have but not blocking.

isaaclab-review-bot · 2026-06-01T08:04:31Z

@@ -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.



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.

Suggested change

## Coding Style

(and remove the old ## Coding style section above, consolidating its content here)

isaaclab-review-bot · 2026-06-01T08:04:31Z

+## Coding Style
+
+### Checks/Asserts
+


Suggested change

In Arena we prefer asserts over if...raises in most cases where such a check represents a coding error

Also: "(i.e)" on the next line should be "(i.e.,)".

isaaclab-review-bot · 2026-06-01T08:04:31Z

+def fetch_smalltable_rows(
+    table_handle: smalltable.Table,
+    keys: Sequence[bytes | str],
+    require_all_keys: bool = False,


This is Google's internal example (smalltable.Table). Consider swapping for a small Arena-relevant snippet to make the guide more concrete for contributors. Even a simple get_observation_space(env_cfg: EnvCfg) -> gym.spaces.Space would anchor the guidance in the project domain.

isaaclab-review-bot · 2026-06-01T08:04:31Z

+          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:


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.

cvolkcvolk · 2026-06-01T09:07:00Z

 - 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


We already have a coding style section just above. Suggestion to remove.

cvolkcvolk · 2026-06-01T09:08:14Z

+
+### Checks/Asserts
+
+In Arena we prefer asserts over if...raises in most cases where such an check represents a coding error


Looks like a duplicate of what we aleady state above on line 40, eg
- Prefer assert condition, "message"overif not condition: raise ValueError("message")for internal invariant checks. (Formatting, imports, and typing are enforced bypre-commit— see.pre-commit-config.yaml.)

cvolkcvolk · 2026-06-01T09:35:53Z

+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


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 "Resist the agent default of long, multi-section descriptions." :)
It's generally recommended to keep the agents.md at or below 150-300 lines of text as it might add to general context rot in other areas. This section alone would add a considerably large part.

Did you have any luck in adding a concise section to the coding style section like

- Docstrings: Google style (https://google.github.io/styleguide/pyguide.html#383-functions-and-methods). - Prefer one line; a 2–3 line paragraph may follow if needed. - Document `Args` and `Returns`, but **not** `Raises`. Omit `Returns` when it only returns None or the summary already covers it.

maybe with a short example?

Eg we refer to the google styleguide (Edit: maybe without linking the URL) and then only state our deltas explicitely

xyao-nv · 2026-06-02T00:33:01Z

+
+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:


google-style -- URL?

Add google style guide docstrings to AGENTS.md

089e7a2

alexmillane requested review from cvolkcvolk, peterd-NV, qianl-nv, viiik-inside, xyao-nv and zhx06 as code owners June 1, 2026 08:02

greptile-apps Bot reviewed Jun 1, 2026

View reviewed changes

isaaclab-review-bot Bot reviewed Jun 1, 2026

View reviewed changes

cvolkcvolk reviewed Jun 1, 2026

View reviewed changes

xyao-nv reviewed Jun 2, 2026

View reviewed changes

isaaclab-arena-review-bot Bot mentioned this pull request Jun 10, 2026

Mesh-based Non-collision Constraints cvolkcvolk/IsaacLab-Arena#10

Open

		In Arena we prefer asserts over if...raises in most cases where such an check represents a coding error
		(i.e) something that shouldn't be recovered from. Asserts are briefer than the exception based counterpart.

		@@ -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.


		### Checks/Asserts

		In Arena we prefer asserts over if...raises in most cases where such an check represents a coding error

Conversation

alexmillane commented Jun 1, 2026

Summary

Detailed description

Uh oh!

greptile-apps Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Flowchart

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

Review: Add google style guide docstrings to AGENTS.md

Key Findings

Uh oh!

isaaclab-review-bot Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

isaaclab-review-bot Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

isaaclab-review-bot Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

isaaclab-review-bot Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

cvolkcvolk Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

cvolkcvolk Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

cvolkcvolk Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

cvolkcvolk Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xyao-nv Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greptile-apps Bot commented Jun 1, 2026 •

edited

Loading

cvolkcvolk Jun 1, 2026 •

edited

Loading