refactor: split UpdateableGuildSetting into focused classes #138
Conversation
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
Resolves GH #89 - Split monolithic UpdateableGuildSetting schema into single-responsibility classes for better maintainability: - BaseGuildSetting: Abstract base class for common validation - GuildModelSetting: Core guild settings (prefix, channels, linking) - GitHubConfigSetting: GitHub integration settings - SOTagsSetting: StackOverflow tags configuration - AllowedUserSetting: User permissions configuration - ForumSetting: Help and showcase forum configuration UpdateableGuildSetting now uses multiple inheritance from all focused classes, maintaining full backwards compatibility with the existing API. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Reviewer's GuideRefactors the monolithic UpdateableGuildSetting schema into a small hierarchy of focused setting classes that share a common BaseGuildSetting, then composes them back into UpdateableGuildSetting via multiple inheritance while updating tests to validate the new structure and maintain backwards compatibility. Class diagram for refactored guild setting schemasclassDiagram
class CamelizedBaseModel
class BaseGuildSetting {
}
class GuildModelSetting {
+str prefix
+int help_channel_id
+int showcase_channel_id
+int thread_channel_id
+bool discussion_linking
+bool comment_linking
+bool pep_linking
}
class GitHubConfigSetting {
+bool discussion_sync
+str github_organization
+str github_repository
}
class SOTagsSetting {
+list~str~ tag_name
}
class AllowedUserSetting {
+int allowed_user_id
}
class ForumSetting {
+bool help_forum
+str help_forum_category
+bool help_thread_auto_close
+int help_thread_auto_close_days
+bool help_thread_close_on_solution
+bool help_thread_sync
+bool showcase_forum
+str showcase_forum_category
+bool showcase_thread_auto_close
+int showcase_thread_auto_close_days
+bool showcase_thread_close_on_solution
+bool showcase_thread_sync
}
class UpdateableGuildSetting {
}
CamelizedBaseModel <|-- BaseGuildSetting
BaseGuildSetting <|-- GuildModelSetting
BaseGuildSetting <|-- GitHubConfigSetting
BaseGuildSetting <|-- SOTagsSetting
BaseGuildSetting <|-- AllowedUserSetting
BaseGuildSetting <|-- ForumSetting
GuildModelSetting <|-- UpdateableGuildSetting
GitHubConfigSetting <|-- UpdateableGuildSetting
SOTagsSetting <|-- UpdateableGuildSetting
AllowedUserSetting <|-- UpdateableGuildSetting
ForumSetting <|-- UpdateableGuildSetting
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
🚅 Deployed to the byte-pr-138 environment in byte
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
BaseGuildSettingdocstring and tests describe it as an abstract base with common validation, but it’s currently a concrete, empty wrapper aroundCamelizedBaseModel; consider either adding actual shared logic/constraints or updating the naming/docs/tests to better reflect its current role. - The new tests that assert exact field counts (e.g.
len(GuildModelSetting.model_fields) == 7) are quite brittle and will fail on any future additive change; consider asserting on the presence/behavior of required fields instead of the total count.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `BaseGuildSetting` docstring and tests describe it as an abstract base with common validation, but it’s currently a concrete, empty wrapper around `CamelizedBaseModel`; consider either adding actual shared logic/constraints or updating the naming/docs/tests to better reflect its current role.
- The new tests that assert exact field counts (e.g. `len(GuildModelSetting.model_fields) == 7`) are quite brittle and will fail on any future additive change; consider asserting on the presence/behavior of required fields instead of the total count.
## Individual Comments
### Comment 1
<location> `services/api/src/byte_api/domain/guilds/schemas.py:197-208` </location>
<code_context>
-# idk
+class UpdateableGuildSetting(
+ GuildModelSetting,
+ GitHubConfigSetting,
+ SOTagsSetting,
+ AllowedUserSetting,
+ ForumSetting,
+):
+ """Allowed settings that admins can update for their guild.
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Multiple inheritance of several Pydantic models could become fragile if overlapping fields/config are introduced later.
This works while these setting models keep distinct field names and compatible `model_config`. If any later introduce overlapping fields or conflicting configs, Pydantic’s MRO can cause subtle issues (e.g., validators/config being unexpectedly overridden). Consider documenting/enforcing non-overlap (via tests or conventions), or using composition (sub-models as fields) if this model grows more complex.
```suggestion
class UpdateableGuildSetting(
GuildModelSetting,
GitHubConfigSetting,
SOTagsSetting,
AllowedUserSetting,
ForumSetting,
):
"""Allowed settings that admins can update for their guild.
This is a composite class that combines all focused setting classes
for backwards compatibility with the existing API.
Important:
All parent setting models **must** keep distinct field names and
compatible ``model_config``. Introducing overlapping field names
or incompatible config may lead to subtle Pydantic MRO issues
(e.g. validators/config being overridden in unexpected ways).
The `_assert_distinct_updateable_guild_setting_fields` guard
below enforces this at import time.
"""
def _assert_distinct_updateable_guild_setting_fields() -> None:
"""Fail fast if any of the composed setting models overlap on fields.
Multiple inheritance of Pydantic models relies on a stable MRO; if
two parents introduce the same field name, behaviour becomes
order-dependent and fragile. This guard makes such changes explicit
by raising at import time.
"""
parents = (
GuildModelSetting,
GitHubConfigSetting,
SOTagsSetting,
AllowedUserSetting,
ForumSetting,
)
seen: dict[str, str] = {}
overlaps: dict[str, list[str]] = {}
for parent in parents:
for field_name in parent.model_fields:
if field_name in seen:
overlaps.setdefault(field_name, [seen[field_name]]).append(parent.__name__)
else:
seen[field_name] = parent.__name__
if overlaps:
details = ", ".join(
f"{name}: {', '.join(sorted(owners))}" for name, owners in sorted(overlaps.items())
)
raise RuntimeError(
"UpdateableGuildSetting parent models must not define overlapping fields. "
f"Overlapping fields detected -> {details}"
)
_assert_distinct_updateable_guild_setting_fields()
```
</issue_to_address>
### Comment 2
<location> `tests/unit/api/test_schemas.py:485-494` </location>
<code_context>
assert data["help_thread_notify_roles"] == []
+class TestBaseGuildSetting:
+ """Tests for BaseGuildSetting schema."""
+
+ def test_base_guild_setting_is_abstract_base(self) -> None:
+ """Test BaseGuildSetting serves as an abstract base class."""
+ from byte_api.lib.schema import CamelizedBaseModel
+
+ assert issubclass(BaseGuildSetting, CamelizedBaseModel)
+ schema = BaseGuildSetting()
+ assert schema.model_dump() == {}
+
+ def test_base_guild_setting_has_docstring(self) -> None:
+ """Test BaseGuildSetting has a docstring."""
+ assert BaseGuildSetting.__doc__ is not None
+
+
</code_context>
<issue_to_address>
**question (testing):** Clarify whether `BaseGuildSetting` is meant to be instantiable and adjust the test accordingly
The docstring calls `BaseGuildSetting` an abstract base class, but this test instantiates it and checks `model_dump() == {}`. If `BaseGuildSetting` should truly be abstract, the test should instead assert that it cannot be instantiated (e.g., via `abc.ABC` or another enforcement). If it is meant to be instantiable, consider renaming the test or updating the docstring so they don’t imply abstract behavior that isn’t enforced, to avoid misleading future readers about its intended use.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
We should prob deprecate SO tags since stackoverflow is effectively dead |
|
Documentation preview will be available shortly at https://jacobcoffee.github.io/byte-docs-preview/138 |
Open
There was a problem hiding this comment.
Pull request overview
This PR successfully refactors the monolithic UpdateableGuildSetting schema into focused, single-responsibility classes as requested in issue #89. The refactoring maintains full backwards compatibility with the existing API by using multiple inheritance.
Changes:
- Created
BaseGuildSettingas an abstract base class for common validation - Split settings into 5 focused classes:
GuildModelSetting,GitHubConfigSetting,SOTagsSetting,AllowedUserSetting, andForumSetting - Maintained
UpdateableGuildSettingas a composite class using multiple inheritance for backwards compatibility - Added comprehensive test coverage for all new classes with inheritance and serialization tests
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| services/api/src/byte_api/domain/guilds/schemas.py | Refactored UpdateableGuildSetting into 6 focused classes (1 base + 5 specialized) with proper inheritance hierarchy and maintained composite class for backwards compatibility |
| tests/unit/api/test_schemas.py | Added comprehensive test classes for all 6 new schema classes, including validation, inheritance, field count, and serialization tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Update BaseGuildSetting docstring to clarify it's a base class, not abstract - Add import-time field overlap guard to prevent MRO issues with multiple inheritance - Replace brittle field count tests with required fields presence tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
UpdateableGuildSettingclass into smaller, more focused classes. #89 - Split monolithicUpdateableGuildSettingschema into single-responsibility classesChanges
BaseGuildSettingGuildModelSettingGitHubConfigSettingSOTagsSettingAllowedUserSettingForumSettingUpdateableGuildSettingnow uses multiple inheritance from all focused classes.Test plan
PATCH /api/guilds/{id}/updateendpoint still works🤖 Generated with Claude Code
Summary by Sourcery
Split the monolithic guild update schema into focused setting classes while preserving the existing composite schema for backwards compatibility.
New Features:
Enhancements:
Tests: