Conversation
Enhanced the CampaignArtifactCounter class to align with refactoring documentation specifications. The core extraction was already complete, but added additional useful features for better usability. Changes to src/artifact_counter.py: - Enhanced ArtifactCounts dataclass with session_ids and narrative_paths lists - Added session_count and narrative_count properties for backward compatibility - Added total_artifacts computed property - Enhanced to_dict() to include new fields - Updated _count_session_artifacts to track session IDs and narrative paths - Added count_sessions() convenience method - Added count_narratives() convenience method - Added get_all_campaigns() to list all campaigns with artifacts - Added get_campaign_summary() for detailed campaign information Changes to tests/test_artifact_counter.py: - Added tests for new properties (session_count, narrative_count, total_artifacts) - Added test for session_ids and narrative_paths tracking - Added tests for count_sessions() convenience method - Added tests for count_narratives() convenience method - Added tests for get_all_campaigns() method - Added tests for get_campaign_summary() method - Added tests to verify session IDs and narrative paths are tracked correctly All changes maintain backward compatibility with existing code. The original implementation already had proper error handling, caching, and testability as required by the refactoring task. Related: docs/refactoring/08-campaign-artifact-counting.md
Summary of ChangesHello @Gambitnl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors and significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the artifact counting logic into a dedicated CampaignArtifactCounter class, which significantly improves modularity, testability, and maintainability. The addition of new features like tracking session IDs and narrative paths, along with convenience methods, enhances the utility of the class. The tests are comprehensive and cover the new functionality well. I've included a few suggestions to further improve code quality by reducing duplication and addressing a potential performance concern.
| def get_all_campaigns(self) -> List[str]: | ||
| """ | ||
| Get list of all campaigns that have artifacts. | ||
|
|
||
| Returns: | ||
| Sorted list of campaign IDs found in the output directory | ||
| """ | ||
| campaigns = set() | ||
|
|
||
| if not self.output_dir.exists(): | ||
| self.logger.warning(f"Output directory not found: {self.output_dir}") | ||
| return [] | ||
|
|
||
| try: | ||
| data_files = list(self.output_dir.glob("**/*_data.json")) | ||
| except Exception as e: | ||
| self.logger.error(f"Failed to glob data files: {e}") | ||
| return [] | ||
|
|
||
| for data_path in data_files: | ||
| try: | ||
| payload = json.loads(data_path.read_text(encoding="utf-8")) | ||
| metadata = payload.get("metadata") or {} | ||
| campaign_id = metadata.get("campaign_id") | ||
| if campaign_id: | ||
| campaigns.add(campaign_id) | ||
| except Exception as e: | ||
| self.logger.debug(f"Skipping {data_path.name}: {e}") | ||
| continue | ||
|
|
||
| return sorted(campaigns) |
There was a problem hiding this comment.
This method performs a file system scan (glob) every time it is called. As noted in the refactoring design document, this can be inefficient for directories with a large number of sessions. While the main count_artifacts method is cached, this discovery method is not. For applications that call this frequently (e.g., to populate a UI dropdown), this could become a performance bottleneck.
Consider adding a caching mechanism to this method, similar to the one used in count_artifacts. A short-lived cache would improve performance for repeated calls without returning overly stale data. You could implement this using a separate cache dictionary and a lock to ensure thread safety.
| return { | ||
| "campaign_id": campaign_id, | ||
| "session_count": counts.sessions, | ||
| "narrative_count": counts.narratives, | ||
| "total_artifacts": counts.total_artifacts, | ||
| "session_ids": counts.session_ids, | ||
| "narrative_paths": [str(p) for p in counts.narrative_paths], | ||
| "error_count": len(counts.errors), | ||
| "errors": counts.errors, | ||
| "last_updated": counts.last_updated.isoformat() if counts.last_updated else None | ||
| } |
There was a problem hiding this comment.
The dictionary being constructed here has significant overlap with the output of ArtifactCounts.to_dict(). This creates code duplication, as much of the logic for serialization is repeated. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, this method should leverage counts.to_dict() and then augment the result.
By centralizing the dictionary creation logic within to_dict(), any future changes to the ArtifactCounts data structure will only need to be updated in one place.
summary = counts.to_dict()
summary["campaign_id"] = campaign_id
summary["errors"] = counts.errors
summary["session_count"] = summary.pop("sessions")
summary["narrative_count"] = summary.pop("narratives")
return summary|
|
||
| assert len(counts.narrative_paths) == 3 | ||
| # Check that all paths are Path objects | ||
| from pathlib import Path |
There was a problem hiding this comment.
This from pathlib import Path statement is redundant. Path is already imported at the top of the file (line 6). According to PEP 8, imports should be at the top of the file to improve readability and avoid unnecessary re-imports. This line can be safely removed.
Updated documentation to reflect completion of campaign artifact counter extraction and enhancement: Changes to docs/refactoring/08-campaign-artifact-counting.md: - Added completion status banner at the top - Added comprehensive Implementation Notes section documenting: - What was already complete (core extraction) - Enhancements added (convenience methods, query methods, tracking) - Usage examples for all new features - Actual vs estimated effort (2 hours vs 9-13 hours) - Success criteria validation - Benefits realized - Integration details - Files modified - Lessons learned Changes to docs/refactoring/README.md: - Marked task #8 as completed with checkmark - Added completion date and status - Added completion notes summarizing enhancements - Updated summary statistics to show 1 of 10 completed - Updated "Last Updated" date to 2025-11-14 The documentation now provides: - Clear completion status for tracking progress - Detailed implementation notes for future reference - Usage examples for developers - Lessons learned for future refactorings
Implemented three improvements based on Gemini code review: 1. Performance: Added caching to get_all_campaigns() - Previously performed filesystem scan on every call - Now uses double-checked locking pattern with TTL-based cache - Added use_cache parameter (default: True) for cache control - Prevents performance bottlenecks when called frequently (e.g., UI dropdowns) 2. Code duplication: Refactored get_campaign_summary() to use to_dict() - Eliminated duplicate dictionary construction logic - Now leverages ArtifactCounts.to_dict() as base - Augments with campaign_id and errors fields - Renames keys for API consistency (sessions -> session_count, etc.) - Follows DRY principle for easier maintenance 3. Code quality: Removed redundant import in tests - Removed inline 'from pathlib import Path' in test_narrative_paths_tracked - Path already imported at module level (line 6) - Improves readability per PEP 8 guidelines Additional improvements: - Updated clear_cache() to also clear campaigns list cache - Enhanced get_cache_stats() to include campaigns_list_cached status - Added comprehensive tests for new caching behavior: - test_get_all_campaigns_caching: Verifies cache works correctly - test_clear_cache_clears_campaigns_list: Verifies cache clearing - test_get_campaign_summary_uses_to_dict: Verifies refactored logic All changes tested and verified with manual integration tests. Related review: gemini-code-assist bot feedback
Resolved merge conflict by acknowledging that both refactorings #6 and #8 have been completed: - Task #6 (Diarizer Complex Method) - completed on main branch - Task #8 (Campaign Artifact Counting) - completed on this branch Changes: - Marked task #6 as completed with status and completion notes - Updated Summary Statistics: 2 of 10 completed (was 1 of 10) - Added actual effort tracking: ~4 hours total - Updated status line to list both completed tasks - Added "Next Priority" guidance for Phase 1 tasks This resolves the upcoming merge conflict when this branch is merged to main or when a PR is created.
…owpfACvL8iZudn6FP6Ews
2 participants
Enhanced the CampaignArtifactCounter class to align with refactoring documentation specifications. The core extraction was already complete, but added additional useful features for better usability.
Changes to src/artifact_counter.py:
Changes to tests/test_artifact_counter.py:
All changes maintain backward compatibility with existing code. The original implementation already had proper error handling, caching, and testability as required by the refactoring task.
Related: docs/refactoring/08-campaign-artifact-counting.md