diff --git a/.cursor/plans/PHASE3-REFACTORING.md b/.cursor/plans/PHASE3-REFACTORING.md new file mode 100644 index 0000000..d999691 --- /dev/null +++ b/.cursor/plans/PHASE3-REFACTORING.md @@ -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 diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index fb268f7..f3a8548 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -371,7 +371,78 @@ GOVMAP_DEFAULT_DEAL_LIMIT=100 ## Future Architecture Evolution -### Phase 2: Data Models (Pydantic) +### Phase 3: Package Refactoring (PLANNED) + +**See `.cursor/plans/PHASE3-REFACTORING.md` for detailed plan** + +Refactor `govmap.py` (1,378 lines) into modular package: + +``` +nadlan_mcp/ +├── __init__.py # Backward compatibility +├── config.py # ✅ Configuration +├── main.py # ✅ Entry point +├── fastmcp_server.py # ✅ MCP tools +└── govmap/ # 📦 NEW PACKAGE + ├── __init__.py # Public API exports + ├── client.py # Core API client (~300 lines) + ├── validators.py # Input validation (~100 lines) + ├── filters.py # Deal filtering (~150 lines) + ├── statistics.py # Statistical calculations (~150 lines) + ├── market_analysis.py # Market analysis (~400 lines) + ├── utils.py # Helper utilities (~100 lines) + └── models.py # Pydantic models (optional) +``` + +**Benefits:** +- ✅ Single Responsibility Principle - each module has one job +- ✅ Easier testing - test modules in isolation +- ✅ Better maintainability - smaller files (100-400 lines) +- ✅ Reusability - import only what you need +- ✅ 100% backward compatible - existing code still works + +**Module Responsibilities:** + +1. **client.py** - Pure API interactions with Govmap + - HTTP requests, rate limiting, response parsing + - No business logic, just API calls + +2. **validators.py** - Input validation + - Address, coordinate, integer, date validation + - Pure functions, clear error messages + +3. **filters.py** - Deal filtering logic + - Property type, rooms, price, area, floor filters + - Composable filter functions + +4. **statistics.py** - Statistical calculations + - Mean, median, percentiles, std dev + - Pure math functions, no I/O + +5. **market_analysis.py** - Market analysis functions + - Activity scoring, investment analysis, liquidity + - Works with data, no API calls + +6. **utils.py** - Shared utilities + - Address matching, text normalization, helpers + - Reusable across modules + +7. **models.py** - Pydantic data models (optional) + - Deal, Address, MarketMetrics, DealStatistics + - Type safety and validation + +**Backward Compatibility:** +```python +# OLD CODE (still works) +from nadlan_mcp import GovmapClient +client = GovmapClient() + +# NEW CODE (also works) +from nadlan_mcp.govmap import GovmapClient +from nadlan_mcp.govmap.filters import filter_deals_by_criteria +``` + +### Phase 4: Pydantic Data Models (Optional) Add structured models for type safety: ```python @@ -386,20 +457,7 @@ class Deal(BaseModel): # ... ``` -### Phase 3: Separation of Concerns - -``` -nadlan_mcp/ -├── api_client.py # Pure API communication -├── analyzers/ -│ ├── market.py # Market analysis logic -│ ├── valuation.py # Valuation helpers -│ └── filtering.py # Deal filtering -├── models.py # Pydantic models -└── fastmcp_server.py # Thin MCP tool definitions -``` - -### Phase 4: Database Layer (Optional) +### Phase 5: Database Layer (Future - Optional) For historical tracking and faster queries: ``` diff --git a/CLAUDE.md b/CLAUDE.md index 6cd0b15..032b3bf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -106,14 +106,16 @@ The codebase follows a three-layer architecture: ## Key Files -- `nadlan_mcp/govmap.py` - Core API client with ~1000 lines of business logic -- `nadlan_mcp/fastmcp_server.py` - MCP tool definitions (7 implemented tools) +- `nadlan_mcp/govmap.py` - Core API client with ~1,378 lines of business logic + - **NOTE:** Will be refactored into package in Phase 3 (see `.cursor/plans/PHASE3-REFACTORING.md`) +- `nadlan_mcp/fastmcp_server.py` - MCP tool definitions (10 implemented tools) - `nadlan_mcp/config.py` - Configuration management - `run_fastmcp_server.py` - Server entry point -- `tests/test_govmap_client.py` - Main test suite +- `tests/test_govmap_client.py` - Main test suite (27 tests) - `USECASES.md` - **Product roadmap and feature status** (essential reading) - `ARCHITECTURE.md` - Detailed system architecture and design decisions - `TASKS.md` - Implementation tasks and progress tracking +- `.cursor/plans/PHASE3-REFACTORING.md` - Detailed refactoring plan for Phase 3 ## Available MCP Tools diff --git a/TASKS.md b/TASKS.md index ec2ab67..b621455 100644 --- a/TASKS.md +++ b/TASKS.md @@ -55,29 +55,52 @@ None - Phase 2 is complete! ## 📋 To-Do (Next Priority) -### Phase 3: Architecture Improvements +### Phase 3: Architecture Improvements & Package Refactoring -#### 3.1 Data Models -- [ ] Create `models.py` with Pydantic models - - [ ] Deal model - - [ ] Address model - - [ ] MarketAnalysis model - - [ ] PropertyValuation model - - [ ] Filter models (DealFilters, etc.) -- [ ] Update functions to use models +**See `.cursor/plans/PHASE3-REFACTORING.md` for detailed implementation plan** + +#### 3.1 Refactor govmap.py into Package Structure +- [ ] **Create package structure** (`nadlan_mcp/govmap/`) + - [ ] Create `govmap/__init__.py` with public API exports + - [ ] Create `govmap/client.py` - Core API client (~300 lines) + - [ ] Create `govmap/validators.py` - Input validation (~100 lines) + - [ ] Create `govmap/filters.py` - Deal filtering (~150 lines) + - [ ] Create `govmap/statistics.py` - Statistical calculations (~150 lines) + - [ ] Create `govmap/market_analysis.py` - Market analysis (~400 lines) + - [ ] Create `govmap/utils.py` - Helper utilities (~100 lines) + +- [ ] **Migrate code by responsibility** + - [ ] Move validation methods to `validators.py` + - [ ] Move filtering logic to `filters.py` + - [ ] Move statistics functions to `statistics.py` + - [ ] Move market analysis to `market_analysis.py` + - [ ] Move utility helpers to `utils.py` + - [ ] Keep only API methods in `client.py` + +- [ ] **Update imports & maintain backward compatibility** + - [ ] Update `nadlan_mcp/__init__.py` for backward compatibility + - [ ] Update `fastmcp_server.py` imports + - [ ] Update `main.py` imports + - [ ] Update test file imports + - [ ] Verify all existing code still works + +- [ ] **Reorganize tests** + - [ ] Create `tests/govmap/` directory + - [ ] Create separate test files for each module + - [ ] Migrate existing tests to new structure + - [ ] Add tests for newly isolated modules + - [ ] Ensure 100% backward compatibility + +#### 3.2 Pydantic Data Models (Optional Enhancement) +- [ ] Create `govmap/models.py` with Pydantic models + - [ ] `Deal` model - Real estate deal + - [ ] `Address` model - Address with coordinates + - [ ] `MarketMetrics` model - Market analysis results + - [ ] `DealStatistics` model - Statistical results + - [ ] `DealFilters` model - Filter criteria +- [ ] Update functions to use/return models (optional) - [ ] Add model validation tests - -#### 3.2 Separation of Concerns -- [ ] Refactor fastmcp_server.py: - - [ ] Move analysis logic to dedicated modules - - [ ] Keep only MCP tool definitions in fastmcp_server.py -- [ ] Create `api_client.py` for pure API interactions -- [ ] Create `analyzers/` package: - - [ ] `analyzers/market.py` - Market analysis functions - - [ ] `analyzers/filtering.py` - Deal filtering logic - - [ ] `analyzers/valuation.py` - Valuation helpers -- [ ] Update imports and dependencies -- [ ] Update tests +- [ ] Add type stubs if needed #### 3.3 LLM-Friendly Tool Design - [ ] Add `summarized_response: bool = False` parameter to all tools @@ -86,6 +109,13 @@ None - Phase 2 is complete! - [ ] Test both modes (structured and summarized) - [ ] Update documentation with examples +#### 3.4 Documentation Updates +- [ ] Update ARCHITECTURE.md with new package structure +- [ ] Update CLAUDE.md with refactored imports +- [ ] Add module-level docstrings to all new files +- [ ] Update README.md if needed +- [ ] Document migration guide for users + ### Phase 4: Testing & Quality #### 4.1 Expand Test Coverage