PR #160 Review: Improve codemap.py with symbol hierarchy

PR #160 Review: Improve codemap.py with symbol hierarchy

Overview

This PR enhances the mapping-codebases skill by adding symbol hierarchy and kind information to the generated code maps. The changes transform the output from simple flat lists of exports to structured, hierarchical representations showing classes with their methods, including signatures where possible.

Changed Files:

• COMPARISON.md (new) - Comparison between TreeSitter and LSP approaches
• mapping-codebases/README.md - Enhanced documentation
• mapping-codebases/scripts/codemap.py - Major refactoring

Strengths

1. Excellent Feature Enhancement

• Symbol Hierarchy: The new Symbol dataclass enables representing nested structures (classes → methods)
• Kind Markers: Clear visual indicators (C, m, f) help distinguish symbol types at a glance
• Signatures: Extracting parameter information provides valuable context for function usage
• Better UX: The structured output is more useful for understanding codebase organization

2. Good Documentation

• COMPARISON.md: Well-written analysis comparing static (TreeSitter) vs dynamic (LSP) approaches. Provides excellent context for why this enhancement was made.
• README.md: Added practical installation instructions with specific version requirements

3. Consistent Refactoring

• Introduced get_node_text() helper to reduce code duplication
• Systematic updates across all language extractors
• Maintained backward compatibility with the overall tool workflow

Issues Found

Critical Issues

1. Python Version Compatibility 🔴

Location: codemap.py:36

children: list['Symbol'] = field(default_factory=list)

Issue: The forward reference syntax list['Symbol'] requires Python 3.10+. For Python 3.7-3.9 compatibility, you need:

from __future__ import annotations

at the top of the file (before other imports).

Impact: Script will fail with SyntaxError on Python 3.9 and earlier.

Fix: Add from __future__ import annotations as the first line after the shebang.

2. Empty Symbol Names 🔴

Location: Multiple extractors

Issue: If parsing fails to find an identifier, an empty name ("") is still added to symbols list. This creates malformed output like:

- **** (C)

Example: codemap.py:71-79 (process_function), codemap.py:81-89 (process_class)

Fix: Check if name is non-empty before creating Symbol:

def process_function(node, kind='function') -> Symbol | None:
    name = ""
    for child in node.children:
        if child.type == 'identifier':
            name = get_node_text(child, source)
            break

    if not name:
        return None

    signature = get_signature(node)
    return Symbol(name=name, kind=kind, signature=signature)

Then filter out None values when appending.

3. Rust Public Symbol Detection Issue 🟡

Location: codemap.py:217-231 (extract_rust)

Issue: The refactored Rust extraction changed from checking attribute_item to checking visibility_modifier directly on items. This might miss some valid public declarations depending on tree-sitter's Rust grammar structure.

Old approach:

elif node.type == 'attribute_item':
    is_pub = False
    for child in node.children:
        if child.type == 'visibility_modifier' and get_text(child) == 'pub':
            is_pub = True

New approach:

elif node.type in ('function_item', 'struct_item', 'enum_item', 'trait_item'):
    is_pub = False
    for child in node.children:
        if child.type == 'visibility_modifier' and get_node_text(child, source) == 'pub':
            is_pub = True

Recommendation: Test with actual Rust code to verify all pub items are detected. The old logic suggests tree-sitter might wrap declarations in attribute_item nodes.

Minor Issues

4. Commented Debug Code 🟡

Locations: Lines 460, 488, 497, 500, 503, 507

Issue: Multiple commented-out debug print statements remain in the code:

# print(f"Error parsing {filepath}: {e}", file=sys.stderr)
# print(f"Processing directory: {dirpath}", file=sys.stderr)

Recommendation: Either remove them entirely or implement proper optional verbose logging:

# Add to main()
parser.add_argument('--verbose', '-v', action='store_true', help='Enable verbose output')
args = parser.parse_args()

# Use throughout
if args.verbose:
    print(f"Processing directory: {dirpath}", file=sys.stderr)

5. Silent Exception Handling 🟡

Location: codemap.py:459-461

Issue: Exception is caught but the error message is commented out:

except Exception as e:
    # print(f"Error parsing {filepath}: {e}", file=sys.stderr)
    return None

Impact: Parsing failures are completely silent, making debugging difficult.

Recommendation: At minimum, log to stderr:

except Exception as e:
    print(f"Warning: Failed to parse {filepath}: {e}", file=sys.stderr)
    return None

6. Inconsistent Private Symbol Filtering 🟢

Location: Various extractors

Observation: Python filters out symbols starting with _ (line 111, 115), but other languages don't filter private/internal symbols (e.g., TypeScript private methods, Java private classes).

Recommendation: Consider adding language-appropriate filtering:

• TypeScript/JavaScript: Filter methods/properties starting with _ or #
• Java: Only include symbols with public modifier (already done for classes, but not methods)
• Go: Only include capitalized symbols (already done)

This is a design choice - current behavior is not wrong, but consistency would be better.

7. TypeScript Signature Extraction Missing 🟢

Location: codemap.py:160 (comment)

Issue: Comment states "Signature extraction is harder in TS due to complexity" and it's not implemented.

Recommendation: At minimum, extract formal_parameters even without type information. The parameter names alone are valuable.

8. Python Method Filtering 🟢

Location: codemap.py:62-69 (visit_class_body)

Issue: The function doesn't filter out methods starting with _, but the top-level function filter does (line 111).

Recommendation: Apply same filtering logic to methods:

def visit_class_body(node) -> list[Symbol]:
    members = []
    for child in node.children:
        if child.type == 'block':
            for subchild in child.children:
                if subchild.type == 'function_definition':
                    method = process_function(subchild, kind='method')
                    if method and not method.name.startswith('_'):
                        members.append(method)
    return members

Testing Recommendations

Required Testing

1. Python Version Compatibility

   • Test on Python 3.7, 3.8, 3.9, 3.10, 3.11
   • Verify the forward reference issue is resolved

2. Empty Name Handling

   • Test with malformed code files that might fail parsing
   • Verify no empty symbol names appear in output

3. Rust Public Symbol Detection

   • Create test file with various pub declaration styles
   • Verify all public items are detected

4. Language Coverage

   • Test with real codebases in each supported language
   • Compare output to actual code structure
   • Verify hierarchy is correct (methods under correct classes)

5. Signature Extraction

   • Test Python functions with various parameter styles (defaults, *args, **kwargs)
   • Verify signatures are extracted correctly

Suggested Test Files

Create a test_files/ directory with samples:

• python_sample.py - Classes with methods, functions, private members
• typescript_sample.ts - Exported classes with methods
• rust_sample.rs - Public functions, structs, implementations
• java_sample.java - Public classes with methods
• malformed.py - Intentionally broken syntax

Recommendations

Must Fix (Before Merge)

1. ✅ Add from __future__ import annotations for Python 3.7+ compatibility
2. ✅ Add validation to prevent empty symbol names
3. ✅ Enable error logging (at least to stderr)

Should Fix (Before Merge)

4. ✅ Test Rust extraction thoroughly and fix if needed
5. ✅ Remove commented debug code or implement proper verbose flag
6. ✅ Apply private method filtering in Python classes

Nice to Have (Can Be Future Work)

7. 🔄 Add TypeScript signature extraction
8. 🔄 Implement consistent private symbol filtering across languages
9. 🔄 Add verbose logging flag
10. 🔄 Create automated tests with sample files

Overall Assessment

Verdict: Approve with Required Changes ⚠️

This PR represents a significant and valuable improvement to the code mapping functionality. The hierarchical symbol representation and kind markers make the generated maps much more useful for understanding codebase structure.

However, the Python compatibility issue must be fixed before merging as it will cause immediate failures on Python 3.9 and earlier. The empty name validation should also be added to prevent malformed output.

With these critical fixes, this PR will be a strong enhancement to the skill.

Summary Checklist

Critical Fixes Required:

• Add from __future__ import annotations for compatibility
• Add empty name validation in symbol creation
• Enable error logging for parse failures

Recommended Fixes:

• Test and verify Rust extraction
• Clean up commented debug code
• Add private method filtering in Python

Testing:

• Test on Python 3.7-3.11
• Test with real codebases in all supported languages
• Verify no empty symbols in output

Documentation:

• README.md updated ✓
• COMPARISON.md added ✓