Getting ready for phase 3

2025-10-24 23:36:58 +03:00
parent f3054c6231
commit 43d031d7c1
4 changed files with 596 additions and 39 deletions
@@ -0,0 +1,467 @@
+# Phase 3: Refactor govmap.py into Package Structure
+
+## Current State Analysis
+
+**File:** `nadlan_mcp/govmap.py`
+- **Lines:** 1,378 lines (too large for single file)
+- **Class:** 1 (`GovmapClient`)
+- **Methods:** 19 total
+  - 7 public API methods
+  - 5 private helper methods
+  - 3 filtering/statistics methods
+  - 3 market analysis methods
+  - 1 validation helper
+
+**Issues:**
+- Single 1,378-line file violates SRP (Single Responsibility Principle)
+- Mixed concerns: API calls, validation, filtering, statistics, market analysis
+- Hard to navigate and maintain
+- Difficult to test individual concerns in isolation
+
+## Proposed Package Structure
+
+```
+nadlan_mcp/
+├── __init__.py                 # Expose GovmapClient for backward compatibility
+├── config.py                   # ✅ Already exists
+├── main.py                     # ✅ Already exists
+├── fastmcp_server.py          # ✅ Already exists (MCP tools)
+└── govmap/                     # 📦 NEW PACKAGE
+    ├── __init__.py             # Export public API
+    ├── client.py               # Core API client (~300 lines)
+    ├── validators.py           # Input validation helpers (~100 lines)
+    ├── filters.py              # Deal filtering logic (~150 lines)
+    ├── statistics.py           # Statistical calculations (~150 lines)
+    ├── market_analysis.py      # Market analysis functions (~400 lines)
+    └── utils.py                # Helper utilities (~100 lines)
+```
+
+## Module Breakdown
+
+### 1. `govmap/client.py` - Core API Client (~300 lines)
+
+**Responsibility:** Pure API interactions with Govmap
+
+**Classes:**
+- `GovmapClient` (core client)
+
+**Methods:**
+- `__init__(config)` - Initialize with config
+- `autocomplete_address(search_text)` - Address search
+- `get_gush_helka(point)` - Block/parcel info
+- `get_deals_by_radius(point, radius)` - Deals within radius
+- `get_street_deals(polygon_id, limit, ...)` - Street deals
+- `get_neighborhood_deals(polygon_id, limit, ...)` - Neighborhood deals
+- `find_recent_deals_for_address(address, years_back, ...)` - Comprehensive search
+- `_rate_limit()` - Private rate limiting helper
+
+**Dependencies:**
+- `config.py` - GovmapConfig
+- `validators.py` - Input validation
+- `requests` - HTTP client
+
+### 2. `govmap/validators.py` - Input Validation (~100 lines)
+
+**Responsibility:** Validate all user inputs before processing
+
+**Functions:**
+- `validate_address(address: str) -> str` - Address validation
+- `validate_coordinates(point: Tuple[float, float]) -> Tuple[float, float]` - Coordinate validation
+- `validate_positive_int(value: int, name: str, max_value: Optional[int]) -> int` - Integer validation
+- `validate_date_range(start_date: str, end_date: str) -> Tuple[str, str]` - Date validation
+- `validate_deal_type(deal_type: int) -> int` - Deal type validation
+
+**Design:**
+- Pure functions (no state)
+- Clear error messages
+- Type hints on all functions
+- Comprehensive docstrings
+
+### 3. `govmap/filters.py` - Deal Filtering (~150 lines)
+
+**Responsibility:** Filter deals by various criteria
+
+**Classes:**
+- `DealFilter` (optional - for complex filtering logic)
+
+**Functions:**
+- `filter_deals_by_criteria(deals, property_type, min_rooms, max_rooms, ...)` - Main filter
+- `filter_by_property_type(deals, property_type)` - Property type filter
+- `filter_by_rooms(deals, min_rooms, max_rooms)` - Room count filter
+- `filter_by_price(deals, min_price, max_price)` - Price range filter
+- `filter_by_area(deals, min_area, max_area)` - Area range filter
+- `filter_by_floor(deals, min_floor, max_floor)` - Floor range filter
+- `extract_floor_number(floor_str: str) -> Optional[int]` - Hebrew floor parser
+
+**Design:**
+- Composable filters (can chain them)
+- Each filter is a pure function
+- Supports OR and AND logic
+
+### 4. `govmap/statistics.py` - Statistical Calculations (~150 lines)
+
+**Responsibility:** Calculate statistics on deal data
+
+**Functions:**
+- `calculate_deal_statistics(deals)` - Comprehensive statistics
+- `calculate_mean(values)` - Mean calculation
+- `calculate_median(values)` - Median calculation
+- `calculate_percentiles(values, percentiles)` - Percentile calculation
+- `calculate_std_dev(values)` - Standard deviation
+- `calculate_coefficient_of_variation(values)` - CV calculation
+- `group_by_property_type(deals)` - Group deals by type
+- `group_by_time_period(deals, period='month')` - Time-based grouping
+
+**Design:**
+- Pure mathematical functions
+- No API calls or I/O
+- Easy to unit test
+- Reusable across different contexts
+
+### 5. `govmap/market_analysis.py` - Market Analysis (~400 lines)
+
+**Responsibility:** Analyze market trends, activity, and investment potential
+
+**Classes:**
+- `MarketAnalyzer` (optional - for stateful analysis)
+
+**Functions:**
+- `calculate_market_activity_score(deals, time_period_months)` - Activity metrics
+- `analyze_investment_potential(deals)` - Investment analysis
+- `get_market_liquidity(deals, time_period_months)` - Liquidity metrics
+- `calculate_price_appreciation_rate(deals)` - Price trends
+- `calculate_volatility_score(deals)` - Volatility analysis
+- `identify_market_trend(deals)` - Trend identification
+
+**Helper Functions:**
+- `_group_deals_by_month(deals)` - Group by month
+- `_group_deals_by_quarter(deals)` - Group by quarter
+- `_calculate_linear_regression(x, y)` - Linear regression
+- `_calculate_trend_direction(values)` - Trend analysis
+
+**Design:**
+- Focused on market metrics
+- No API calls (works with data)
+- Returns structured dictionaries
+- MCP-friendly output format
+
+### 6. `govmap/utils.py` - Helper Utilities (~100 lines)
+
+**Responsibility:** Shared utility functions
+
+**Functions:**
+- `is_same_building(search_address, deal_address)` - Address matching
+- `normalize_hebrew_text(text)` - Hebrew text normalization
+- `parse_date(date_str)` - Date parsing
+- `format_currency(amount)` - Currency formatting
+- `calculate_distance(point1, point2)` - Coordinate distance
+- `extract_year_month(date_str)` - Extract year-month from date
+
+**Design:**
+- Pure utility functions
+- No external dependencies (except standard library)
+- Reusable across modules
+
+### 7. `govmap/__init__.py` - Package Exports
+
+**Purpose:** Clean public API
+
+```python
+"""
+Govmap API Client Package
+
+Provides access to Israeli government real estate data via the Govmap API.
+"""
+
+from .client import GovmapClient
+from .filters import filter_deals_by_criteria
+from .statistics import calculate_deal_statistics
+from .market_analysis import (
+    calculate_market_activity_score,
+    analyze_investment_potential,
+    get_market_liquidity,
+)
+
+__all__ = [
+    "GovmapClient",
+    "filter_deals_by_criteria",
+    "calculate_deal_statistics",
+    "calculate_market_activity_score",
+    "analyze_investment_potential",
+    "get_market_liquidity",
+]
+```
+
+### 8. `nadlan_mcp/__init__.py` - Update for Backward Compatibility
+
+```python
+"""
+Israel Real Estate MCP
+
+A Python-based Mission Control Program to interact with the Israeli government's
+public real estate data API (Govmap).
+"""
+
+# Backward compatibility - expose GovmapClient at top level
+from .govmap import GovmapClient
+
+__version__ = "1.0.0"
+__all__ = ["GovmapClient"]
+```
+
+## Migration Strategy
+
+### Phase 3.1: Create Package Structure ✅
+1. Create `nadlan_mcp/govmap/` directory
+2. Create all module files with proper structure
+3. Add `__init__.py` exports
+
+### Phase 3.2: Move Code by Responsibility ✅
+1. **validators.py** - Extract validation methods
+   - `_validate_address()` → `validate_address()`
+   - `_validate_coordinates()` → `validate_coordinates()`
+   - `_validate_positive_int()` → `validate_positive_int()`
+
+2. **utils.py** - Extract utility functions
+   - `_is_same_building()` → `is_same_building()`
+   - `_extract_floor_number()` → `extract_floor_number()`
+
+3. **filters.py** - Extract filtering logic
+   - `filter_deals_by_criteria()` → Keep as main function
+   - Break into composable filter functions
+
+4. **statistics.py** - Extract statistical functions
+   - `calculate_deal_statistics()` → Keep as main function
+   - `_calculate_std_dev()` → `calculate_std_dev()`
+   - Add new statistical helpers
+
+5. **market_analysis.py** - Extract market analysis
+   - `calculate_market_activity_score()` → Keep as is
+   - `analyze_investment_potential()` → Keep as is
+   - `get_market_liquidity()` → Keep as is
+
+6. **client.py** - Core API methods remain
+   - `autocomplete_address()`
+   - `get_gush_helka()`
+   - `get_deals_by_radius()`
+   - `get_street_deals()`
+   - `get_neighborhood_deals()`
+   - `find_recent_deals_for_address()`
+
+### Phase 3.3: Update Imports ✅
+1. Update `fastmcp_server.py` imports
+2. Update `main.py` imports
+3. Update test files imports
+4. Ensure backward compatibility in `nadlan_mcp/__init__.py`
+
+### Phase 3.4: Update Tests ✅
+1. Create new test files for each module:
+   - `tests/govmap/test_client.py`
+   - `tests/govmap/test_validators.py`
+   - `tests/govmap/test_filters.py`
+   - `tests/govmap/test_statistics.py`
+   - `tests/govmap/test_market_analysis.py`
+   - `tests/govmap/test_utils.py`
+2. Move existing tests to appropriate files
+3. Add new tests for isolated modules
+
+### Phase 3.5: Add Pydantic Models (Optional Enhancement) ✅
+1. Create `nadlan_mcp/govmap/models.py`
+2. Define data models:
+   - `Deal` - Real estate deal
+   - `Address` - Address with coordinates
+   - `MarketMetrics` - Market analysis results
+   - `DealStatistics` - Statistical results
+3. Update functions to use/return models
+
+## Benefits of Refactoring
+
+### 1. Maintainability
+- ✅ Each module has single responsibility
+- ✅ Easy to find and modify code
+- ✅ Smaller files (100-400 lines each)
+
+### 2. Testability
+- ✅ Test each module in isolation
+- ✅ Mock dependencies easily
+- ✅ Faster test execution
+- ✅ Better test organization
+
+### 3. Reusability
+- ✅ Import only what you need
+- ✅ Use filters independently of client
+- ✅ Compose functions as needed
+
+### 4. Extensibility
+- ✅ Add new filters without touching client
+- ✅ Add new analyses independently
+- ✅ Clear extension points
+
+### 5. Team Collaboration
+- ✅ Multiple developers can work on different modules
+- ✅ Clearer code ownership
+- ✅ Reduced merge conflicts
+
+## Backward Compatibility
+
+**Guarantee:** All existing code continues to work
+
+```python
+# OLD CODE (still works)
+from nadlan_mcp import GovmapClient
+
+client = GovmapClient()
+deals = client.find_recent_deals_for_address("סוקולוב 38 חולון")
+
+# NEW CODE (also works)
+from nadlan_mcp.govmap import GovmapClient
+from nadlan_mcp.govmap.filters import filter_deals_by_criteria
+from nadlan_mcp.govmap.statistics import calculate_deal_statistics
+
+client = GovmapClient()
+deals = client.find_recent_deals_for_address("סוקולוב 38 חולון")
+filtered = filter_deals_by_criteria(deals, property_type="דירה", min_rooms=3)
+stats = calculate_deal_statistics(filtered)
+```
+
+## Implementation Checklist
+
+### Phase 3.1: Package Structure
+- [ ] Create `nadlan_mcp/govmap/` directory
+- [ ] Create `govmap/__init__.py`
+- [ ] Create `govmap/client.py` (empty)
+- [ ] Create `govmap/validators.py` (empty)
+- [ ] Create `govmap/filters.py` (empty)
+- [ ] Create `govmap/statistics.py` (empty)
+- [ ] Create `govmap/market_analysis.py` (empty)
+- [ ] Create `govmap/utils.py` (empty)
+
+### Phase 3.2: Move Validation Code
+- [ ] Move validation functions to `validators.py`
+- [ ] Update imports in `client.py`
+- [ ] Add tests for validators
+- [ ] Verify backward compatibility
+
+### Phase 3.3: Move Utility Code
+- [ ] Move utility functions to `utils.py`
+- [ ] Update imports in `client.py`
+- [ ] Add tests for utils
+- [ ] Verify backward compatibility
+
+### Phase 3.4: Move Filtering Code
+- [ ] Move filtering logic to `filters.py`
+- [ ] Create composable filter functions
+- [ ] Update imports in `client.py`
+- [ ] Add tests for filters
+- [ ] Verify backward compatibility
+
+### Phase 3.5: Move Statistics Code
+- [ ] Move statistical functions to `statistics.py`
+- [ ] Break into smaller functions
+- [ ] Update imports in `client.py`
+- [ ] Add tests for statistics
+- [ ] Verify backward compatibility
+
+### Phase 3.6: Move Market Analysis Code
+- [ ] Move market analysis to `market_analysis.py`
+- [ ] Organize helper functions
+- [ ] Update imports in `client.py`
+- [ ] Add tests for market analysis
+- [ ] Verify backward compatibility
+
+### Phase 3.7: Finalize Client
+- [ ] Keep only API methods in `client.py`
+- [ ] Update all imports
+- [ ] Add comprehensive docstrings
+- [ ] Verify all functionality works
+
+### Phase 3.8: Update Imports Everywhere
+- [ ] Update `fastmcp_server.py`
+- [ ] Update `main.py`
+- [ ] Update `nadlan_mcp/__init__.py`
+- [ ] Update all test files
+- [ ] Run all tests - ensure they pass
+
+### Phase 3.9: Optional Enhancements
+- [ ] Add Pydantic models (`models.py`)
+- [ ] Add type stubs (`.pyi` files)
+- [ ] Add `py.typed` marker
+- [ ] Update documentation
+
+### Phase 3.10: Documentation
+- [ ] Update ARCHITECTURE.md with new structure
+- [ ] Update CLAUDE.md with package info
+- [ ] Update README.md if needed
+- [ ] Add module-level docstrings
+- [ ] Update TASKS.md
+
+## Testing Strategy
+
+### Unit Tests (New)
+```
+tests/govmap/
+├── __init__.py
+├── test_client.py           # API client tests
+├── test_validators.py       # Validation tests
+├── test_filters.py          # Filtering tests
+├── test_statistics.py       # Statistics tests
+├── test_market_analysis.py  # Market analysis tests
+└── test_utils.py            # Utility tests
+```
+
+### Integration Tests
+- Keep existing integration tests
+- Ensure they work with new structure
+- Add new integration tests for full workflows
+
+### Backward Compatibility Tests
+```python
+def test_backward_compatibility():
+    """Ensure old import style still works."""
+    from nadlan_mcp import GovmapClient
+    client = GovmapClient()
+    assert client is not None
+```
+
+## Success Criteria
+
+- ✅ All existing tests pass
+- ✅ No breaking changes to public API
+- ✅ Each module < 500 lines
+- ✅ 100% backward compatible
+- ✅ All imports work correctly
+- ✅ Documentation updated
+- ✅ Type hints on all functions
+- ✅ Comprehensive docstrings
+
+## Timeline Estimate
+
+- **Phase 3.1:** Package structure (1 hour)
+- **Phase 3.2-3.7:** Code migration (4-6 hours)
+- **Phase 3.8:** Import updates (1 hour)
+- **Phase 3.9:** Testing (2-3 hours)
+- **Phase 3.10:** Documentation (1-2 hours)
+
+**Total:** ~10-14 hours of development time
+
+## Risks & Mitigation
+
+### Risk 1: Breaking Changes
+**Mitigation:** Maintain backward compatibility in `nadlan_mcp/__init__.py`
+
+### Risk 2: Import Cycles
+**Mitigation:** Careful dependency design, validators/utils have no dependencies
+
+### Risk 3: Test Failures
+**Mitigation:** Migrate tests incrementally, run after each module
+
+### Risk 4: Lost Functionality
+**Mitigation:** Comprehensive test coverage before refactoring
+
+## Notes
+
+- This refactoring is **non-breaking** - all existing code continues to work
+- Focus on **separation of concerns** - each module has one job
+- **Pure functions** where possible - easier to test and reason about
+- **Incremental migration** - move one module at a time, test thoroughly
+- **Documentation first** - update docs as you refactor