Refactor memory and storage modules #24
Conversation
Split monolithic storage.py (942 lines) into focused modules under storage/ directory: - storage/base.py - BaseStorage with common helpers and path management - storage/messages.py - Message storage operations (add, get, search, stats) - storage/nodes.py - Node registry and name mapping operations - storage/adverts.py - Advertisement and network event storage - storage/__init__.py - MeshBotStorage facade for backward compatibility Benefits: - Better separation of concerns - Easier to maintain and test individual components - Reduced code duplication - Cleaner architecture with single responsibility per module - Maintains backward compatibility through facade pattern The memory.py interface remains unchanged, seamlessly using the new modular storage backend.
jinglemansweep
deleted the
claude/refactor-memory-storage-01Lj1dR9u85UFZzE36V1iKet
branch
There was a problem hiding this comment.
Pull request overview
This PR refactors the monolithic storage.py (942 lines) into a modular architecture under the storage/ directory. The refactoring introduces a three-tier class hierarchy: BaseStorage provides common path management helpers, NodeStorage extends it with node operations, and AdvertStorage further extends with advertisement tracking. MessageStorage independently handles message operations. A MeshBotStorage facade in __init__.py maintains backward compatibility by delegating to specialized modules.
Key changes:
- Splits storage responsibilities into focused, single-purpose modules
- Introduces inheritance hierarchy: BaseStorage → NodeStorage → AdvertStorage
- MessageStorage operates independently from node/advert storage
- Maintains identical public API through facade pattern
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/meshbot/storage/base.py |
Provides common path management, directory creation, and helper methods shared across storage modules |
src/meshbot/storage/messages.py |
Handles all message CRUD operations including add, get, search, and statistics |
src/meshbot/storage/nodes.py |
Manages node registry, name mappings, and node metadata operations |
src/meshbot/storage/adverts.py |
Extends NodeStorage with advertisement logging and network event tracking |
src/meshbot/storage/__init__.py |
Facade providing unified MeshBotStorage interface for backward compatibility |
src/meshbot/storage.py |
Deleted - original monolithic storage implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.data_path.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Create subdirectories | ||
| self.nodes_dir = self.data_path / "nodes" | ||
| self.channels_dir = self.data_path / "channels" | ||
| self.nodes_dir.mkdir(exist_ok=True) | ||
| self.channels_dir.mkdir(exist_ok=True) | ||
|
|
||
| async def initialize(self) -> None: | ||
| """Initialize storage (create data directory if needed).""" | ||
| try: | ||
| self.data_path.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
Directory creation in __init__ is duplicated in the initialize() method at line 31. Since directories are already created in __init__, the initialize() method's directory creation is redundant. Consider removing lines 20, 25-26 from __init__ and only creating directories in initialize(), or documenting why both are needed.
| self.data_path.mkdir(parents=True, exist_ok=True) | |
| # Create subdirectories | |
| self.nodes_dir = self.data_path / "nodes" | |
| self.channels_dir = self.data_path / "channels" | |
| self.nodes_dir.mkdir(exist_ok=True) | |
| self.channels_dir.mkdir(exist_ok=True) | |
| async def initialize(self) -> None: | |
| """Initialize storage (create data directory if needed).""" | |
| try: | |
| self.data_path.mkdir(parents=True, exist_ok=True) | |
| # Set subdirectory paths | |
| self.nodes_dir = self.data_path / "nodes" | |
| self.channels_dir = self.data_path / "channels" | |
| async def initialize(self) -> None: | |
| """Initialize storage (create data directory if needed).""" | |
| try: | |
| self.data_path.mkdir(parents=True, exist_ok=True) | |
| self.nodes_dir.mkdir(exist_ok=True) | |
| self.channels_dir.mkdir(exist_ok=True) |
| self._message_storage = MessageStorage(data_path) | ||
| self._advert_storage = AdvertStorage(data_path) |
There was a problem hiding this comment.
Both MessageStorage and AdvertStorage instantiate BaseStorage via inheritance, causing the same directories (data_path, nodes_dir, channels_dir) to be created twice in their respective __init__ methods. Consider extracting shared initialization to avoid redundant directory creation operations.
| await self._advert_storage.initialize() | ||
|
|
There was a problem hiding this comment.
Calling initialize() on both storage instances logs 'Storage initialized' twice with the same path and attempts directory creation twice. Since AdvertStorage inherits from NodeStorage which inherits from BaseStorage, and MessageStorage also inherits from BaseStorage, both call BaseStorage.initialize() which performs identical operations. Consider calling initialize() only once or coordinating initialization to avoid redundant operations.
| await self._advert_storage.initialize() |
Split monolithic storage.py (942 lines) into focused modules under storage/ directory:
Benefits:
The memory.py interface remains unchanged, seamlessly using the new modular storage backend.