Skip to content

Conversation

@ezynda3
Copy link
Contributor

@ezynda3 ezynda3 commented Dec 19, 2025

Description

Add comprehensive docstrings for exported annotation-related functions in mcp/resources.go and mcp/utils.go.

This is based on PR #664 but with improved documentation that avoids duplication (as noted in the review comments on that PR).

Type of Change

  • Documentation update

Checklist

  • My code follows the code style of this project
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the documentation accordingly

Additional Information

Functions updated:

  • WithAnnotations: Describes return type and field initialization behavior
  • WithLastModified: Consolidates timestamp format documentation (fixes duplication issue from 📝 Add docstrings to feat/annotations-mcp-2025-11-25 #664)
  • WithTemplateAnnotations: Describes return type and field initialization behavior
  • ValidateISO8601Timestamp: Documents empty string handling and return behavior
  • ParseAnnotations: Adds comprehensive docstring explaining parsing behavior for each field

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Fixed potential crashes related to annotation initialization in resource handling.
  • Improvements

    • Enhanced timestamp validation for RFC3339 format compliance across resources.
    • Added new annotation parsing capabilities for improved data processing and consistency.

✏️ Tip: You can customize this high-level summary in your review settings.

Improve documentation for exported functions in mcp/resources.go and mcp/utils.go:
- WithAnnotations: describe return type and field initialization
- WithLastModified: consolidate timestamp format documentation
- WithTemplateAnnotations: describe return type and field initialization
- ValidateISO8601Timestamp: document empty string handling and return behavior
- ParseAnnotations: add comprehensive docstring for parsing behavior
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

Walkthrough

Updates resource option helpers and annotations handling to ensure nil-safety and add annotation parsing. Modifies WithAnnotations, WithLastModified, and WithTemplateAnnotations to initialize Annotations before use, adds ParseAnnotations function to parse annotation data from maps, and strengthens RFC3339 validation documentation across timestamp handling.

Changes

Cohort / File(s) Summary
Resource Option Helpers
mcp/resources.go
Added nil-safety guards to WithAnnotations, WithLastModified, and WithTemplateAnnotations to initialize Annotations before field assignment; updated ValidateISO8601Timestamp to enforce RFC3339 validation while treating empty strings as valid; enhanced documentation for all affected functions to clarify initialization and timestamp formatting requirements.
Annotation Parsing Utility
mcp/utils.go
Added new public function ParseAnnotations that converts annotation data from a map into an Annotations struct, parsing Priority as \*float64, filtering Audience to include only RoleUser and RoleAssistant, and extracting LastModified from string values.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • mcp/resources.go: Review nil-initialization logic in WithAnnotations, WithLastModified, and WithTemplateAnnotations to ensure proper guard conditions and RFC3339 validation behavior
  • mcp/utils.go: Verify ParseAnnotations implementation handles map parsing correctly and matches expected Annotations struct semantics, particularly Priority pointer handling and Audience role filtering

Possibly related PRs

Suggested labels

type: enhancement

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add docstrings for annotation-related functions' accurately summarizes the main change—adding documentation to annotation-related functions across two files.
Description check ✅ Passed The description follows the template structure with all required sections completed: Description, Type of Change, and Checklist are all properly filled out with relevant details about the docstring additions.
Docstring Coverage ✅ Passed Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch add-docstrings-annotations

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
mcp/resources.go (1)

59-69: Consider validating the timestamp format.

The function initializes Annotations correctly and is well-documented. However, it accepts any string without validation, even though ValidateISO8601Timestamp exists in the same file (lines 120-130). Invalid timestamps will only be detected later when consumed.

🔎 Optional: Add validation

If you want to enforce format validation at construction time:

 func WithLastModified(timestamp string) ResourceOption {
 	return func(r *Resource) {
+		if err := ValidateISO8601Timestamp(timestamp); err != nil {
+			// Option: return early, log warning, or panic depending on error handling strategy
+			return
+		}
 		if r.Annotations == nil {
 			r.Annotations = &Annotations{}
 		}
 		r.Annotations.LastModified = timestamp
 	}
 }

Note: This changes the error handling model (functional options typically don't return errors), so this may not align with your design. Current approach allows flexibility for callers to validate separately if needed.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bd8762d and 9f2ddde.

📒 Files selected for processing (2)
  • mcp/resources.go (4 hunks)
  • mcp/utils.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing

Files:

  • mcp/utils.go
  • mcp/resources.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test
🔇 Additional comments (4)
mcp/utils.go (1)

539-573: LGTM! Well-documented and implemented annotation parsing.

The ParseAnnotations function is well-designed with comprehensive documentation. The implementation correctly:

  • Returns nil for nil input
  • Parses priority as a pointer (allowing distinction between "not set" and "zero")
  • Filters audience to valid roles (RoleUser and RoleAssistant)
  • Handles LastModified as a string

The nil-safety checks and use of the cast library for type conversions are appropriate.

mcp/resources.go (3)

45-57: LGTM! Clear documentation and nil-safe implementation.

The documentation accurately describes the behavior, and the implementation properly initializes Annotations when nil before setting fields. The priority is correctly stored as a pointer.


107-118: LGTM! Documentation and implementation are correct.

The function properly initializes Annotations when nil and sets all three fields as documented. The behavior matches WithAnnotations for resources.


120-130: LGTM! Clear validation function with proper documentation.

The documentation accurately describes the RFC3339/ISO 8601 validation behavior, including the handling of empty strings as valid. The implementation is straightforward and correct.

@ezynda3 ezynda3 merged commit 1db477e into main Dec 19, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants