Skip to content

Align CI and dev tools with movement repository standards#2

Closed
sfmig wants to merge 13 commits into
mainfrom
ci/align-with-movement-standards
Closed

Align CI and dev tools with movement repository standards#2
sfmig wants to merge 13 commits into
mainfrom
ci/align-with-movement-standards

Conversation

@sfmig

@sfmig sfmig commented Sep 16, 2025

Copy link
Copy Markdown
Member

Summary

This PR aligns the beekeeper repository's CI checks, dev tools, and contributing guidelines with the standards used in the neuroinformatics-unit/movement repository.

Changes Made

🐍 Python Version Support

  • Updated minimum Python version from 3.9 to 3.11
  • Updated classifiers to support Python 3.11 and 3.12
  • Modernized test matrix in GitHub Actions

🔧 Formatting and Linting

  • Removed black completely from the toolchain
  • Now using ruff for both linting and formatting (following movement's approach)
  • Updated CONTRIBUTING.md to reflect ruff-only workflow

🪝 Pre-commit Hooks

  • Updated to match movement's configuration
  • Added codespell for spell checking
  • Disabled check-manifest for simplicity (appropriate for beekeeper's simple structure)
  • All hooks now use latest versions

🚀 GitHub Actions

  • Updated workflow name to "Tests" (matching movement)
  • Added merge_group trigger for better CI flow
  • Updated all neuroinformatics-unit actions to v2 (latest)
  • Improved caching strategy with OS-specific keys
  • Added codecov token support for coverage reporting

📝 Documentation

  • Updated CONTRIBUTING.md to reflect new development workflow
  • Added guidance for handling auto-fixed changes from hooks
  • Removed references to black formatter

Test Plan

  • All pre-commit hooks pass locally
  • Documentation builds successfully
  • Configuration tested against movement repository patterns
  • Maintains beekeeper's simplicity while adopting best practices

Notes

The configuration is now minimal but consistent with movement repository standards, appropriate for beekeeper's simpler application structure. This ensures consistency across neuroinformatics-unit projects while keeping the tooling lightweight.

🤖 Generated with Claude Code

sfmig and others added 12 commits November 24, 2024 17:25
This commit modernizes the beekeeper application by removing legacy
WAZP functionality and focusing specifically on video metadata management.

Key changes:
- Fix obsolete Dash API (app.run_server -> app.run)
- Remove unused DLC/pose estimation code (~200 lines)
- Update deprecated pandas method (applymap -> map)
- Remove legacy dependencies (tables, blosc2, h5py references)
- Add proper error handling for missing configurations
- Update test fixtures to use tmp_path instead of external data
- Modernize documentation with current functionality
- Add comprehensive user and developer workflow guides

The application now provides a clean, focused interface for managing
video metadata through YAML files, spreadsheet imports, and interactive
table editing.

🤖 Generated with Claude Code (claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
…d appears first.

- Add integration tests to verify that the key field is consistently the first column in the metadata table.
- Update unit tests to confirm that the metadata table creation respects the specified column order based on the key field.
- Add module docstrings to all core beekeeper modules
- Fix mypy type errors with proper type annotations and ignores
- Update pre-commit hooks to use latest ruff (v0.12.11) and mypy (v1.17.1)
- Fix ruff configuration format (tool.ruff.lint section)
- Resolve SIM210 warnings (unnecessary True/False expressions)
- Add missing function docstrings and parameter descriptions
- Fix numerous docstring formatting issues (D205, D417, D400, D415)
- Apply comprehensive code formatting with ruff and black
- Fix codespell errors (doesn't, triggered)
- Add noqa: C901 comment for complex function
- Fix wheel dependency issue for check-manifest
- Fix RST documentation formatting issues

All pre-commit checks now pass: mypy, ruff, black, check-manifest, codespell.
Reduced linting errors from 54 to 0 in core codebase.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Consolidated overlapping "Quick Start" and "Getting Started" sections into single "Getting Started" section
- Removed redundant instructions and streamlined installation steps
- Updated badges to match movement repository style: kept License, CI, Ruff, and pre-commit badges
- Maintained clear "Core Operations" section as requested

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
…ation to point to the new repository at sfmig/beekeeper.
This commit updates the development workflow configuration to match
the standards used in the neuroinformatics-unit/movement repository:

- Update Python support to 3.11+ (from 3.9+) to align with current standards
- Remove black formatter, now using ruff for both linting and formatting
- Update GitHub Actions workflow to use latest action versions (v2)
- Add merge_group trigger and improve caching strategy
- Update Python test matrix to 3.11, 3.12 with modern OS selection
- Add codecov token support for coverage reporting
- Enhance pre-commit hooks with codespell (check-manifest disabled for simplicity)
- Update CONTRIBUTING.md to reflect ruff-only formatting approach
- Fix codespell configuration in pyproject.toml
- Apply automated linting fixes (strict=False in zip call)

The configuration is now minimal but consistent with movement repository
standards, appropriate for beekeeper's simpler application structure.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude

claude Bot commented Sep 17, 2025

Copy link
Copy Markdown

Code Review: PR #2 - Align CI and dev tools with movement repository standards 🔍

I have conducted a thorough review of this PR that aligns the beekeeper repository with the movement repository standards. Here is my detailed feedback:

Strengths

  1. Excellent modernization effort: Upgrading from Python 3.9 to 3.11/3.12 is a good move for performance and security
  2. Streamlined tooling: Consolidating on ruff for both linting and formatting simplifies the dev workflow
  3. Improved CI/CD: Addition of merge_group trigger and better caching strategy will improve CI efficiency
  4. Clean refactoring: The renaming from WAZP to beekeeper is handled consistently across all files
  5. Better test fixtures: Moving from external data dependencies to pytest fixtures with temporary directories is a cleaner approach

🔍 Code Quality Issues to Address

1. Incomplete Error Messages

In tests/test_integration/test_common_layout.py:44-45, there is a dangling string that appears to be from incomplete refactoring. The string " in the browser console!" should be concatenated with the assertion message above.

2. Test Coverage Gaps

While test improvements are good, I notice:

  • Limited tests for the new utils.py functions beyond df_from_metadata_yaml_files
  • Missing integration tests for spreadsheet upload functionality
  • No explicit tests for error handling in metadata export

⚠️ Potential Issues

  1. Path Resolution: The transition in test fixtures might cause issues on different platforms. Consider using pathlib.Path consistently throughout.

  2. Error Handling: In beekeeper/utils.py, the new utility functions could benefit from more robust error handling for empty/malformed YAML files, missing required fields, and type validation failures.

🔧 Performance Considerations

  1. File I/O Operations: The metadata reading functions scan directories multiple times. Consider caching directory listings for better performance.

  2. DataFrame Operations: Column reordering in table creation could be optimized using DataFrame.reindex().

🔒 Security Considerations

  1. YAML Loading: Good use of yaml.safe_load() throughout - maintains security ✅

  2. Path Validation: Consider adding validation for file paths to prevent potential security issues when handling user-uploaded configs

  3. Input Sanitization: The spreadsheet upload functionality should validate file types and sizes

📝 Documentation

The updates to CONTRIBUTING.md are comprehensive and helpful. The new developer workflow section is particularly valuable.

Overall Assessment

This is a well-executed modernization PR that successfully aligns the codebase with movement repository standards. The changes are logical, improve developer experience, and maintain backward compatibility where it matters.

Recommendation: ✅ Approve with minor changes - The incomplete error message issue should be fixed. Other items can be tracked as follow-up issues.

Great work on this comprehensive update! 🎉


Review conducted using Claude Code

@claude

claude Bot commented Sep 18, 2025

Copy link
Copy Markdown

Pull Request Review: Align CI and dev tools with movement repository standards

Thank you for this significant refactoring effort! This PR successfully transitions from WAZP to beekeeper while modernizing the development toolchain. Here's my detailed review:

Strengths

Code Quality & Best Practices

  • Excellent modularization: Clear separation of concerns with dedicated modules for app initialization, pages, callbacks, and utilities
  • Proper typing: Good use of type hints throughout the codebase (e.g., beekeeper/utils.py:13-15)
  • Clean architecture: The Dash multi-page pattern with separate callback modules is well-implemented
  • Consistent formatting: Ruff configuration with line length 79 enforces consistency

CI/CD Improvements

  • Modern toolchain: Updated from Black to Ruff for unified linting/formatting
  • Better Python support: Dropped Python 3.9-3.10, focusing on 3.11-3.12
  • Enhanced CI: Added merge_group trigger and improved caching strategy
  • Comprehensive pre-commit hooks: Added codespell, pygrep-hooks, and better mypy configuration

Documentation

  • Clear README: Excellent user-facing documentation with step-by-step instructions
  • Well-documented functions: Comprehensive docstrings following NumPy style
  • Updated CONTRIBUTING.md: Good developer workflow documentation

⚠️ Areas for Improvement

1. Path Handling Security

In beekeeper/callbacks/metadata.py:752, files are written based on user input without path validation:

with open(pl.Path(video_dir) / yaml_filename, "w") as yamlf:

Recommendation: Add path traversal protection to ensure files are only written within the intended directory.

2. Error Handling

Several places could benefit from more robust error handling:

  • beekeeper/utils.py:54-61: YAML parsing without exception handling
  • Missing validation for malformed project configurations

Recommendation: Add try-except blocks with user-friendly error messages.

3. Test Coverage

While tests exist, coverage could be expanded:

  • No tests for error conditions (malformed YAML, missing files)
  • Limited integration tests for the full upload→edit→export workflow
  • No tests for edge cases (empty dataframes, special characters in filenames)

4. Performance Considerations

  • beekeeper/utils.py:52-66: Building dataframes in a loop with concatenation could be inefficient for large datasets
  • Consider using list comprehension and single concatenation

📝 Minor Issues

  1. Unused imports: Check if all dash components imported are actually used
  2. Magic numbers: Consider extracting constants like page size (25) in metadata.py
  3. TODO comments: Several TODOs remain in the code (e.g., utils.py:92-94)

🔒 Security Notes

  • ✅ Using yaml.safe_load() instead of yaml.load() - good!
  • ✅ No use of eval(), exec(), or pickle
  • ⚠️ Consider adding input validation for uploaded files (size limits, content validation)

📊 Overall Assessment

This is a solid refactoring that successfully modernizes the codebase and development workflow. The transition from WAZP to beekeeper is clean, and the code quality improvements are substantial. The main areas for improvement are around error handling, test coverage, and input validation.

Recommendation: Approve with minor revisions. The security and error handling improvements should be addressed in a follow-up PR.

Great work on this significant refactoring! 🎉

@sfmig

sfmig commented Sep 29, 2025

Copy link
Copy Markdown
Member Author

superseded by PR #1

@sfmig sfmig closed this Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant