style: address linting and typing issues #29
Conversation
Reviewer's Guide by SourceryThis pull request introduces linting with Ruff and type-checking with Mypy to enforce code quality and consistency across the project. The changes include adding type annotations to existing Python files, updating the CI pipeline to include Ruff and Mypy checks, and integrating Lefthook for pre-push checks. The implementation focuses on improving code quality, type safety, and maintaining consistent coding standards. Architecture diagram for CI pipeline with Ruff and Mypygraph TD;
A[CI Pipeline] -->|Run on push/pull_request| B[Build Job];
B --> C[Checkout Code];
B --> D[Set up Python 3.11];
B --> E[Install Poetry];
B --> F[Lint with Ruff];
B --> G[Type check with Mypy];
B --> H[Run security checks with Bandit];
B --> I[Install test dependencies];
F -->|If successful| G;
Class diagram for updated converters with type annotationsclassDiagram
class AbstractConverter {
<<abstract>>
+convert_to_wrroc(data: Dict~str, Any~) Dict~str, Any~
+convert_from_wrroc(wrroc_data: Dict~str, Any~) Dict~str, Any~
}
class TESConverter {
+convert_to_wrroc(tes_data: Dict~str, Any~) Dict~str, Any~
+convert_from_wrroc(wrroc_data: Dict~str, Any~) Dict~str, Any~
}
class WESConverter {
+convert_to_wrroc(wes_data: Dict~str, Any~) Dict~str, Any~
+convert_from_wrroc(wrroc_data: Dict~str, Any~) Dict~str, Any~
}
class ConverterManager {
+convert_tes_to_wrroc(tes_data: Dict~str, Any~) Dict~str, Any~
+convert_wes_to_wrroc(wes_data: Dict~str, Any~) Dict~str, Any~
}
AbstractConverter <|-- TESConverter
AbstractConverter <|-- WESConverter
TESConverter <.. ConverterManager
WESConverter <.. ConverterManager
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Karanjot786 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
uniqueg
changed the title
There was a problem hiding this comment.
Looks mostly good to me. Two minor comments:
- Please do the upgrade/modifications of the CI workflow in a separate PR and remove from here. The new PR should be merged before this one. Also make sure to remove the commented out lines. Readd them only later when you actually add tests (obviously uncommented then).
- Ignore the Sourcery comments for this PR. They may be useful to consider in future PRs though.
Hi @uniqueg, Thanks for the feedback!
As suggested, I will create a separate PR specifically for the upgrade and modifications to the CI workflow. Once that’s merged, I’ll remove the changes from this PR. I'll make sure to remove the commented-out lines and re-add them later when the tests are included.
Got it! I'll ignore the Sourcery comments for this PR and will take them into account for future PRs where relevant. |
|
Thanks @Karanjot786! #30 is approved now and can be merged. Please:
|
|
I have temporarily adjusted the |
|
Hi @uniqueg, I've updated the docstrings as per your feedback. All functions and methods now include "Args", "Returns", and other relevant sections where applicable. I've ensured consistency across the codebase. As you suggested, I have not added type hints in this PR and plan to address them in an upcoming PR. Please let me know if there's anything else you'd like me to adjust. |
There was a problem hiding this comment.
Thanks, looks great. One thing before we can merge: Please only include "Raises" sections in functions/methods that explicitly raise errors (most don't). You don't wanna describe errors that the code may implicitly raise, because you'll never see the end of that.
|
I've updated the docstrings to remove the "Raises" sections from functions and methods that do not explicitly raise exceptions. This should make the documentation cleaner and more focused. |
Description:
This PR introduces linting with Ruff and type-checking with Mypy to enforce code quality and consistency across the project. The key changes are outlined below:
Added Support for Ruff and Mypy
Linting with Ruff:
Type-Checking with Mypy:
Code Updates
Type Annotations Added:
crategen/converters/abstract_converter.pycrategen/converters/utils.pycrategen/converters/wes_converter.pycrategen/converters/tes_converter.pycrategen/converter_manager.pycrategen/cli.pyconvert_to_iso8601for ISO 8601 conversions.AbstractConverter,WESConverter,TESConverter, andConverterManagerclasses.CI Pipeline Enhancements
Continuous Integration Updates:
.github/workflows/ci.ymlto:Lefthook Integration
Pre-Push Hooks:
lefthook.ymlconfiguration to enforce pre-push Ruff checks.Files Changed
CI and Hooks Configuration:
.github/workflows/ci.yml: Updated to run Ruff and Mypy checks during CI.lefthook.yml: Added to configure Ruff for pre-push linting.Codebase Updates:
crategen/converters/abstract_converter.py: Added type annotations to abstract methods.crategen/converters/utils.py: Added type annotations toconvert_to_iso8601.crategen/converters/wes_converter.py: Added type annotations and fixed untyped function calls.crategen/converters/tes_converter.py: Added type annotations and fixed untyped function calls.crategen/converter_manager.py: Added type annotations to methods.crategen/cli.py: Added type annotations and updated function signatures.Summary by Sourcery
Add linting with Ruff and type-checking with Mypy to improve code quality and consistency. Update CI pipeline to include these checks and integrate Lefthook for pre-push linting enforcement. Enhance codebase with type annotations for better maintainability.
New Features:
Enhancements:
CI:
Chores: