Implement guild data endpoints

[knowlen]

Summary

• Implements the remaining 4 guild data methods, bringing guild API coverage to 100%
• Adds flexible guild lookup supporting both ID and name/server combinations
• Includes comprehensive testing and documentation

Changes

• New Methods:

  • get_guilds() - List guilds with pagination and server filtering
  • get_guild() - Flexible lookup by ID or name/server/region
  • get_guild_attendance() - Retrieve guild raid attendance data
  • get_guild_members() - Get guild member roster

• Implementation Details:

  • Added 4 new GraphQL queries to queries.graphql
  • Created parameter builder for guild attendance with proper defaults
  • Implemented flexible validation for get_guild() method
  • Updated guild mixin with factory-based method registration

• Testing:

  • Added 12 unit tests covering all new methods
  • Added 10 integration tests with real API validation
  • All tests passing with proper mocking and error handling

• Documentation:

  • Updated guild-data.md with examples for all new methods
  • Added parameter tables and usage patterns
  • Included error handling examples

Impact

• API coverage increased from ~83% to ~90% (37/41 methods)
• Guild API section now 100% complete (6/6 methods)
• Only User Data (3 methods) and Progress Race (1 method) remain

Test Plan

• Unit tests pass
• Integration tests pass
• Pre-commit hooks pass
• Documentation updated
• API coverage metrics updated

[claude]

Claude finished @knowlen's task —— View job

---

Code Review Complete ✅

I've thoroughly reviewed this substantial PR that implements the remaining guild data endpoints. Here's my comprehensive feedback:

Review Tasks:

• Read repository setup and guidelines
• Review core implementation (guild mixin, method factory)
• Analyze new GraphQL queries and parameter builders
• Check parameter validation and error handling
• Review test coverage and quality
• Assess security and performance considerations
• Check documentation completeness
• Provide comprehensive feedback

---

Overall Assessment: Excellent ⭐

This is a well-architected, thoroughly tested implementation that follows the existing codebase patterns excellently. The code quality is high with strong attention to security, performance, and maintainability.

Detailed Review

✅ Code Quality & Architecture

Strengths:

• Excellent factory pattern: The method factory system in method_factory.py is clean and extensible
• Consistent mixin architecture: Guild methods follow the established pattern used by other mixins
• Proper separation of concerns: Clear division between generated code, business logic, and parameter handling
• Type safety: Comprehensive type hints and Pydantic models ensure runtime safety

Minor suggestions:

• The get_guild() method validation logic (lines 127-147 in guild.py) is quite dense - consider extracting to a dedicated validation function
• Consider caching compiled regex patterns in method_factory.py (already done with _CAMEL_TO_SNAKE_PATTERN - good!)

✅ Security Considerations

Excellent security practices:

• Input validation: Robust validation in validators.py with length limits (MAX_GUILD_NAME_LENGTH, MAX_SERVER_SLUG_LENGTH)
• API key sanitization: The sanitize_api_key_from_error() function prevents accidental key exposure
• DoS protection: String length limits prevent potential DoS attacks via large inputs
• SQL injection prevention: Using parameterized GraphQL queries correctly

No security concerns identified.

✅ Performance Considerations

Well optimized:

• Compiled regex caching: _CAMEL_TO_SNAKE_PATTERN is compiled once and reused
• Efficient parameter building: Parameter builders avoid unnecessary object creation
• Lazy validation: UNSET pattern prevents unnecessary processing
• Proper async/await usage: Non-blocking I/O implementation

Minor optimization opportunity:

• Consider memoizing method creation in the factory to avoid re-compilation if the same methods are created multiple times (though this may be premature optimization)

✅ Test Coverage - Comprehensive

Outstanding test coverage:

• 12 unit tests covering all new methods with proper mocking
• 10 integration tests with real API validation
• Edge case coverage: Tests for validation errors, not-found scenarios, and pagination
• Proper test organization: Clear separation between unit and integration tests

Test quality highlights:

• Mock usage is correct and non-intrusive
• Integration tests validate actual API response structures
• Error scenarios are properly tested

✅ Parameter Validation & Error Handling

Robust implementation:

• Flexible validation: The get_guild() method handles both ID and name-based lookups elegantly
• Clear error messages: Validation errors provide helpful context (esologs/mixins/guild.py:130-147)
• Proper parameter mapping: Snake_case Python params correctly map to camelCase GraphQL (param_builders.py:338-347)
• Default values: Sensible defaults for pagination in build_guild_attendance_params()

✅ GraphQL Implementation

Well-structured queries:

• Consistent field selection: All queries request appropriate nested data
• Proper parameterization: Variable usage follows GraphQL best practices
• Complete coverage: Queries cover all necessary fields for each operation

⚠️ Minor Issues Found

1. Type inconsistency in guild.py:91: The self: Any parameter could be more specific. Consider using a protocol or the actual client type.

2. Potential import cycle: The import of ValidationError inside the get_guild() method (line 110) suggests a potential circular import. Consider moving this to the top-level imports.

3. Documentation: The generated model classes lack docstrings, though this is likely intentional for generated code.

✅ Documentation Quality

Comprehensive documentation:

• Updated API reference: guild-data.md provides clear examples and parameter tables
• Architecture documentation: Updates to architecture.md explain the new patterns
• Changelog: Properly documented changes
• Inline documentation: Method docstrings are clear and helpful

Recommendations

High Priority

1. Extract validation logic: Consider moving the complex validation in get_guild() to a dedicated function in validators.py
2. Fix import placement: Move the ValidationError import to the top level to avoid potential circular imports

Low Priority

1. Consider type refinement: Use more specific types instead of Any where possible
2. Performance monitoring: Consider adding optional timing/metrics for the new endpoints

Final Verdict

This is an exemplary PR that demonstrates:

• ✅ Excellent code quality and architecture
• ✅ Comprehensive testing (22 new tests!)
• ✅ Strong security practices
• ✅ Thorough documentation
• ✅ Consistent with existing patterns
• ✅ Performance-conscious implementation

The implementation successfully brings guild API coverage to 100% while maintaining the high quality standards of the codebase. The factory pattern and mixin architecture make future API additions straightforward.

Recommendation: Approve 🚀

---