feat: Implement comprehensive guild data API endpoints (5 methods)

[knowlen]

Summary

• Implements ALL guild data API endpoints (5 methods total)
• Brings API coverage from ~76% to ~88% (37/42 methods implemented)
• Guild data section is now COMPLETE ✅

Changes

Guild Methods Implemented

1. get_guild_by_id(guild_id) - Get guild information by ID
2. get_guild(...) - Flexible guild lookup by ID or name/server combination
3. get_guilds(...) - List/search guilds with pagination and server filtering
4. get_guild_attendance(...) - Get guild raid attendance data
5. get_guild_members(...) - Get guild member list with pagination

Additional Changes

• Added comprehensive GraphQL queries for all guild operations
• Implemented smart routing in get_guild() for flexible lookups
• Added extensive test coverage for all guild endpoints
• Updated documentation with guild data examples
• Fixed import consistency (using absolute imports)
• Added validation strategy documentation

Test plan

• Unit tests pass
• Integration tests pass
• Documentation examples work correctly
• No breaking changes to existing API
• All 5 guild methods tested and working

Notes

• Originally merged to main by mistake in PR Implement guild data endpoints #25, now correctly targeting dev
• Reviewer feedback addressed:
  • Import consistency fixed (absolute imports)
  • Validation strategy documented

[claude]

Claude finished @knowlen's task —— View job

---

PR Review: Guild Endpoints Implementation

I've completed a comprehensive review of this guild endpoints implementation. Overall, this is a solid addition with good architecture and extensive test coverage.

Review Summary

• Review core guild implementation files
• Analyze GraphQL queries and generated code
• Review test coverage and quality
• Check for security and performance issues
• Review documentation and examples
• Provide comprehensive feedback

---

✅ Strengths

1. Comprehensive Implementation

• Complete guild API coverage: Successfully implements all major guild endpoints (get_guild_by_id, get_guilds, get_guild, get_guild_attendance, get_guild_members)
• Consistent architecture: Follows existing mixin pattern in esologs/mixins/guild.py:49-111
• Method factory usage: Proper use of factory patterns for code generation in esologs/method_factory.py:375-378

2. Excellent Test Coverage

• Unit tests: Comprehensive mocking and validation tests in tests/unit/test_guild_methods.py
• Integration tests: Real API testing with proper error handling in tests/integration/test_guild_api.py
• Documentation tests: Validates all code examples actually work in tests/docs/test_guild_data_examples.py

3. Security Considerations

• Input validation: Strong parameter validation with length limits in esologs/validators.py:17-40
• API key sanitization: Prevents key exposure in error messages in esologs/validators.py:42-61
• DoS protection: String length limits and positive integer validation

4. Type Safety & Code Quality

• Proper type hints: All methods have comprehensive type annotations
• Pydantic models: Generated models with proper field aliases and validation
• Error handling: Appropriate use of ValidationError for parameter issues

---

⚠️ Areas for Improvement

1. PR Size Management

• Issue: 3,081 additions across 66 files makes comprehensive review challenging
• Recommendation: Consider breaking large features into smaller, focused PRs in future
• Impact: Medium - affects review quality and merge risk

2. Import Consistency

Some inconsistencies in import patterns:

# esologs/mixins/guild.py:5 - Good absolute import
from esologs._generated.base_model import UNSET, UnsetType

# But mixing patterns in other files
from .async_base_client import AsyncBaseClient  # Relative
from esologs._generated.base_model import UNSET  # Absolute

Recommendation: Standardize on absolute imports for better maintainability.

3. Parameter Validation Inconsistency

The get_guild() method has comprehensive validation (esologs/mixins/guild.py:112-185), but some generated methods lack equivalent validation:

# guild.py has detailed validation
if not has_guild_id and not has_complete_name_info:
    raise ValidationError("Must provide either guild_id OR guild_name...")

# But generated methods rely mainly on GraphQL schema validation

Recommendation: Consider adding validation layer for all public methods.

4. Generated Code Optimization

Some repetitive patterns in generated files could be optimized:

# Multiple files have identical model_rebuild() patterns
GetGuilds.model_rebuild()
GetGuildsGuildData.model_rebuild()
GetGuildsGuildDataGuilds.model_rebuild()
# ... etc

Note: This may be a code generation tool limitation rather than a code issue.

---

🔧 Minor Issues

1. Documentation Examples

• Some hardcoded values in examples (guild_id = 3468) could be more generic
• Consider adding more error handling examples in documentation

2. GraphQL Query Organization

The queries in queries.graphql:171-901 are well-structured, but consider grouping related queries with comments for better maintainability.

3. Method Factory Configuration

The configuration in esologs/method_factory.py:375-378 could include more descriptive docstrings for the guildId parameter mapping.

---

🚀 Performance Considerations

Positive Aspects:

• Efficient GraphQL queries: Well-designed queries that fetch only needed fields
• Proper pagination: All list methods support pagination with reasonable defaults
• Parameter building: Efficient parameter builders in esologs/param_builders.py:333-360

No Performance Issues Found:

• No N+1 query patterns
• Appropriate use of async/await
• Efficient data model structures

---

🛡️ Security Assessment

Excellent Security Practices:

• Input sanitization: Comprehensive validation functions
• API key protection: Error message sanitization
• DoS prevention: String length and pagination limits
• Type safety: Strong typing prevents injection issues

No Security Concerns Found:

• No SQL injection risks (GraphQL-based)
• No exposed secrets in code
• Proper parameter validation throughout

---

📋 Test Quality Assessment

Comprehensive Coverage:

• Unit tests: 444 lines covering all public methods
• Integration tests: 338 lines with real API interactions
• Documentation tests: 136 lines validating examples work
• Proper mocking: Clean separation of unit and integration tests

Test Structure Strengths:

• Good use of pytest fixtures
• Proper async test handling
• Clear test organization and naming
• Appropriate use of mocks for external dependencies

---

✅ Final Recommendation

APPROVE with minor suggestions for future improvements.

This PR successfully implements a complete, well-tested guild API with:

• ✅ Solid architecture following existing patterns
• ✅ Comprehensive test coverage (unit, integration, docs)
• ✅ Strong security considerations
• ✅ Good documentation with working examples
• ✅ Proper type safety and error handling

The identified issues are minor and don't block merging. They can be addressed in follow-up PRs if desired.

Great work on maintaining code quality while delivering a substantial feature! 🎉

---

[knowlen]

@claude Thank you for the thorough review! I've addressed both concerns:

1. Import Consistency ✅

Fixed the relative imports in esologs/mixins/init.py to use absolute imports:

Before

from .guild import GuildMixin

After

from esologs.mixins.guild import GuildMixin

All handwritten code now uses consistent absolute imports. The generated files still use relative
imports as they're auto-generated, but we maintain consistency in our own code.

2. Parameter Validation Strategy ✅

Added documentation to clarify our validation approach:

• Module-level documentation in guild.py explaining that methods mapping directly to single
  GraphQL operations rely on GraphQL schema validation
• Method-level documentation for get_guild() explaining why it has comprehensive client-side
  validation (it's a convenience method that routes to different GraphQL queries)

The validation difference is intentional:

• get_guild_by_id, get_guilds, get_guild_attendance, get_guild_members → Direct GraphQL mappings →
  GraphQL validation
• get_guild() → Routes between get_guild_by_id and get_guild_by_name → Needs client-side
  validation to determine routing

Additional Context

This PR actually implements all 5 guild endpoints, not just get_guild_by_id:

1. get_guild_by_id(guild_id) - Get guild by ID
2. get_guild(...) - Flexible lookup by ID or name/server
3. get_guilds(...) - List/search guilds with pagination
4. get_guild_attendance(...) - Get raid attendance data
5. get_guild_members(...) - Get member list

This brings our API coverage from ~76% to ~88% (37/42 methods) and completes the guild data
section! 🎉

All changes have been pushed. Let me know if you have any other concerns!

[claude]

Claude finished @knowlen's task —— View job

---

Follow-up Review: Addressing Feedback ✅

Thanks for the quick response and fixes! I've verified the changes you made to address my feedback.

Todo List

• Review the import consistency fixes in esologs/mixins/init.py
• Check parameter validation documentation added to esologs/mixins/guild.py
• Provide final acknowledgment and assessment of the fixes

---

✅ Excellent Response to Feedback!

Both concerns have been properly addressed:

1. Import Consistency Fixed ✅

Verified in esologs/mixins/__init__.py:7-11 - all imports now use consistent absolute import style:

from esologs.mixins.character import CharacterMixin
from esologs.mixins.game_data import GameDataMixin
from esologs.mixins.guild import GuildMixin
# etc.

This creates a clear, maintainable import pattern for all handwritten code.

2. Parameter Validation Strategy Documented ✅

Excellent documentation added to esologs/mixins/guild.py:4-9 explaining the validation approach:

• Module-level: Clear explanation of why different methods use different validation strategies
• Method-level: get_guild() method (lines 127-130) explains why it has comprehensive client-side validation

The distinction makes perfect sense:

• Direct GraphQL mappings → GraphQL schema validation
• Convenience routing method → Client-side validation for routing logic

3. Comprehensive Implementation Noted ✅

Thank you for clarifying the full scope! You're absolutely right - this PR implements all 5 guild endpoints:

1. get_guild_by_id(guild_id) - Direct guild lookup
2. get_guild(...) - Smart routing convenience method
3. get_guilds(...) - List/search with pagination
4. get_guild_attendance(...) - Raid attendance data
5. get_guild_members(...) - Member list with pagination

Bringing API coverage from ~76% to ~88% (37/42 methods) is substantial progress! 🎉

---

🎯 Final Assessment

STRONG APPROVE - All feedback addressed excellently. The changes demonstrate:

• ✅ Attention to code quality and consistency
• ✅ Clear documentation of design decisions
• ✅ Comprehensive feature implementation
• ✅ Professional response to review feedback

No further concerns. This is ready to merge! 🚀

Great work on both the implementation and the thoughtful response to feedback.

---