LCORE-1374: Contributing guide#1224
Merged
tisnik merged 1 commit intolightspeed-core:mainfrom Feb 25, 2026
Merged
Conversation
Contributor
WalkthroughA new "Function Standards" section is added to two documentation files (CONTRIBUTING.md and docs/contributing_guide.md) under Code style, introducing guidelines for documentation, type annotations, naming conventions, async functions, and error handling. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CONTRIBUTING.md (1)
246-261: Same content issues asdocs/contributing_guide.md.The
Optional[Type]/ modern-syntax inconsistency, the missing cross-reference to "Docstrings style", and the missing terminal periods flagged indocs/contributing_guide.mdapply equally here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 246 - 261, The CONTRIBUTING.md fragment repeats the same issues as docs/contributing_guide.md: inconsistent type syntax (mix of modern union syntax `str | int` and legacy `Optional[Type]`), missing cross-reference to the "Docstrings style" section, and several list items lacking terminal periods; update the text to consistently use one typing style (prefer modern `X | Y` or explicitly state policy and convert `Optional[Type]` to `Type | None` if choosing modern syntax), add a cross-reference/link to the "Docstrings style" section, and ensure all bullet/list entries (e.g., "Use `typing_extensions.Self` for model validators", "Union types: `str | int`", "Optional: `Optional[Type]`", "Use snake_case...", "Use `async def`...", "Use FastAPI `HTTPException`...") end with terminal periods for consistency.
🧹 Nitpick comments (2)
CONTRIBUTING.md (1)
236-262: Duplicate content will drift — consider a single source of truth.The entire "Function Standards" block (lines 236–262) is identical to the one in
docs/contributing_guide.md. Keeping two copies in sync manually is error-prone. A common approach is to keep the canonical content in one file and replace the other with a brief summary and a link.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 236 - 262, Remove the duplicated "### Function Standards" section from CONTRIBUTING.md and replace it with a short one-paragraph summary that points to the canonical version in docs/contributing_guide.md (e.g., "See the full Function Standards in docs/contributing_guide.md") so the single source of truth is the file docs/contributing_guide.md; ensure the replacement references the "### Function Standards" heading and retains any necessary linking text but does not re-duplicate the full block.docs/contributing_guide.md (1)
238-240: Add a cross-reference to the existing "Docstrings style" section.The "Documentation" subsection is intentionally terse, but it should point readers to the detailed "Docstrings style" section below (which covers
Args,Returns,Raises, and a full example). Without the link, contributors may not discover the fuller guidance.✏️ Suggested update
#### Documentation -All functions require docstrings with brief descriptions +All functions require docstrings with brief descriptions. See the +[Docstrings style](`#docstrings-style`) section for format and examples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/contributing_guide.md` around lines 238 - 240, Update the terse "Documentation" subsection to include a cross-reference/link to the detailed "Docstrings style" section: modify the paragraph that reads "All functions require docstrings with brief descriptions" so it ends with a pointer such as "See the 'Docstrings style' section below for required Args, Returns, Raises and examples." Ensure the visible section title string "Docstrings style" is used verbatim so readers can easily find the full guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contributing_guide.md`:
- Around line 246-248: The docs currently mix modern PEP 604 union syntax (`str
| int`) with the legacy `Optional[Type]`; update the guidance to use modern
optional unions everywhere by replacing the `Optional[Type]` recommendation with
`Type | None` and note that `typing_extensions.Self` guidance remains unchanged;
ensure the examples and bullet list consistently prefer the `X | Y` form for
both unions and optionals.
- Around line 240-261: Add missing terminal periods to the prose sentences in
the new section: add a period at the end of the lines for "All functions require
docstrings with brief descriptions", the "Type annotations" introductory line,
the "Use snake_case with descriptive, action-oriented names (get_, validate_,
check_)" line, and the "Use async def for I/O operations and external API calls"
line so that they end with full stops; locate these within the section that
contains the headings "All functions require docstrings with brief
descriptions", "#### Type annotations", "#### Naming conventions", and "####
Async functions" and update the sentences to include a trailing period for
consistency with surrounding documentation style.
---
Duplicate comments:
In `@CONTRIBUTING.md`:
- Around line 246-261: The CONTRIBUTING.md fragment repeats the same issues as
docs/contributing_guide.md: inconsistent type syntax (mix of modern union syntax
`str | int` and legacy `Optional[Type]`), missing cross-reference to the
"Docstrings style" section, and several list items lacking terminal periods;
update the text to consistently use one typing style (prefer modern `X | Y` or
explicitly state policy and convert `Optional[Type]` to `Type | None` if
choosing modern syntax), add a cross-reference/link to the "Docstrings style"
section, and ensure all bullet/list entries (e.g., "Use `typing_extensions.Self`
for model validators", "Union types: `str | int`", "Optional: `Optional[Type]`",
"Use snake_case...", "Use `async def`...", "Use FastAPI `HTTPException`...") end
with terminal periods for consistency.
---
Nitpick comments:
In `@CONTRIBUTING.md`:
- Around line 236-262: Remove the duplicated "### Function Standards" section
from CONTRIBUTING.md and replace it with a short one-paragraph summary that
points to the canonical version in docs/contributing_guide.md (e.g., "See the
full Function Standards in docs/contributing_guide.md") so the single source of
truth is the file docs/contributing_guide.md; ensure the replacement references
the "### Function Standards" heading and retains any necessary linking text but
does not re-duplicate the full block.
In `@docs/contributing_guide.md`:
- Around line 238-240: Update the terse "Documentation" subsection to include a
cross-reference/link to the detailed "Docstrings style" section: modify the
paragraph that reads "All functions require docstrings with brief descriptions"
so it ends with a pointer such as "See the 'Docstrings style' section below for
required Args, Returns, Raises and examples." Ensure the visible section title
string "Docstrings style" is used verbatim so readers can easily find the full
guidance.
Comment on lines
+240
to
+261
| All functions require docstrings with brief descriptions | ||
|
|
||
| #### Type annotations | ||
|
|
||
| Use complete type annotations for parameters and return types | ||
|
|
||
| - Use `typing_extensions.Self` for model validators | ||
| - Union types: `str | int` (modern syntax) | ||
| - Optional: `Optional[Type]` | ||
|
|
||
| #### Naming conventions | ||
|
|
||
| Use snake_case with descriptive, action-oriented names (get_, validate_, check_) | ||
|
|
||
| #### Async functions | ||
|
|
||
| Use `async def` for I/O operations and external API calls | ||
|
|
||
| #### Error handling | ||
|
|
||
| - Use FastAPI `HTTPException` with appropriate status codes for API endpoints | ||
| - Handle `APIConnectionError` from Llama Stack where appropriate |
Contributor
There was a problem hiding this comment.
Prose lines in the new section are missing terminal periods.
Lines 240, 244, 252, and 256 each end a sentence without a full stop. This is inconsistent with the surrounding documentation style.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contributing_guide.md` around lines 240 - 261, Add missing terminal
periods to the prose sentences in the new section: add a period at the end of
the lines for "All functions require docstrings with brief descriptions", the
"Type annotations" introductory line, the "Use snake_case with descriptive,
action-oriented names (get_, validate_, check_)" line, and the "Use async def
for I/O operations and external API calls" line so that they end with full
stops; locate these within the section that contains the headings "All functions
require docstrings with brief descriptions", "#### Type annotations", "####
Naming conventions", and "#### Async functions" and update the sentences to
include a trailing period for consistency with surrounding documentation style.
Comment on lines
+246
to
+248
| - Use `typing_extensions.Self` for model validators | ||
| - Union types: `str | int` (modern syntax) | ||
| - Optional: `Optional[Type]` |
Contributor
There was a problem hiding this comment.
Optional[Type] contradicts the adjacent modern-syntax recommendation.
The guide promotes PEP 604 str | int syntax (modern) on line 247, but then prescribes the legacy Optional[Type] form on line 248. Since the project requires Python 3.12+, both union and optional should use the modern union syntax (Type | None) for consistency.
✏️ Suggested update
- Use `typing_extensions.Self` for model validators
-- Union types: `str | int` (modern syntax)
-- Optional: `Optional[Type]`
+- Union types: use `str | int` (PEP 604 modern syntax)
+- Optional values: use `Type | None` instead of `Optional[Type]`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Use `typing_extensions.Self` for model validators | |
| - Union types: `str | int` (modern syntax) | |
| - Optional: `Optional[Type]` | |
| - Use `typing_extensions.Self` for model validators | |
| - Union types: use `str | int` (PEP 604 modern syntax) | |
| - Optional values: use `Type | None` instead of `Optional[Type]` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contributing_guide.md` around lines 246 - 248, The docs currently mix
modern PEP 604 union syntax (`str | int`) with the legacy `Optional[Type]`;
update the guidance to use modern optional unions everywhere by replacing the
`Optional[Type]` recommendation with `Type | None` and note that
`typing_extensions.Self` guidance remains unchanged; ensure the examples and
bullet list consistently prefer the `X | Y` form for both unions and optionals.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
LCORE-1374: Contributing guide
Type of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit