Skip to content

refactor: first iteration of fixing mypy complaints #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jjjermiah
Copy link
Contributor

@jjjermiah jjjermiah commented Mar 16, 2025

NOTE: This PR depends on #58 to be merged first

This PR fixes all the mypy complaints within the repo

This allows future PRs to start addressing any discrepencies between the types annotated so far and all the concrete implementations throughout the interface plugins

Summary by CodeRabbit

  • Chores
    • Enhanced the automation pipeline with a new type-checking step for improved quality control.
  • Refactor
    • Improved type safety and clarity across various components, including error handling, logging, and plugin management.
  • Documentation
    • Added detailed documentation and utility methods to the SettingsEnumBase class for better clarity.

These behind-the-scenes improvements help ensure a smoother, more dependable experience for our users.

Copy link

coderabbitai bot commented Mar 16, 2025

📝 Walkthrough

Walkthrough

The pull request updates the GitHub Actions workflow by adding a Mypy type-checking step to the quality-control job. In the codebase, several files are modified to enhance type annotations and clarify method signatures. The WorkflowError class is updated with new optional attributes, and the PluginRegistryBase class is made generic with the introduction of a type variable. The SettingsEnumBase class is also enhanced with new utility methods. Overall, these changes focus on improving type safety and clarity throughout the codebase.

Changes

File(s) Change Summary
.github/workflows/test.yml Added Mypy type-checking step to the quality-control job, executing pixi run --environment dev type-check.
src/snakemake_interface_common/exceptions.py Added optional attributes (lineno, snakefile, rule) to WorkflowError; updated type annotations in format_arg, __init__, and _get_spec.
src/snakemake_interface_common/logging.py Added type hints to get_logger function, specifying its return as a logging.Logger; moved the logging import inside a TYPE_CHECKING block.
src/snakemake_interface_common/plugin_registry/__init__.py Introduced generics for PluginRegistryBase via a new type variable; added explicit type annotations and return types in methods.
src/snakemake_interface_common/plugin_registry/attribute_types.py Added type annotations to methods in AttributeType, specifying return types.
src/snakemake_interface_common/plugin_registry/plugin.py Enhanced type annotations and introduced generics; updated method signatures and variable types for improved type clarity.
src/snakemake_interface_common/rules.py Removed the @property decorator from the snakefile method in RuleInterface and changed its return type from Path to str.
src/snakemake_interface_common/settings.py Enhanced SettingsEnumBase class with additional documentation and utility methods for handling settings enumerations.
src/snakemake_interface_common/utils.py Enhanced type safety by adding explicit type annotations to functions and methods, including not_iterable, methods within lazy_property, lutime, fmt_time, and lchmod.

Possibly related PRs

  • feat: setup pixi, move to src layout, run ruff format once #58: The changes in the main PR, which involve adding a Mypy type-checking step to the GitHub Actions workflow, are related to the retrieved PR that emphasizes the enforcement of type annotations and addresses type-related issues across multiple files, including those in the same codebase.

Suggested reviewers

  • bsmith89
  • johanneskoester

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ea66427 and abd814d.

📒 Files selected for processing (1)
  • src/snakemake_interface_common/plugin_registry/plugin.py (13 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

  • src/snakemake_interface_common/plugin_registry/plugin.py
🧬 Code Definitions (1)
src/snakemake_interface_common/plugin_registry/plugin.py (3)
src/snakemake_interface_common/plugin_registry/__init__.py (1)
  • register_cli_args (64-68)
src/snakemake_interface_common/plugin_registry/tests.py (1)
  • validate_settings (26-26)
src/snakemake_interface_common/rules.py (1)
  • name (7-7)
🪛 Ruff (0.8.2)
src/snakemake_interface_common/plugin_registry/plugin.py

271-271: Use is and is not for type comparisons, or isinstance() for isinstance checks

(E721)

🔇 Additional comments (23)
src/snakemake_interface_common/plugin_registry/plugin.py (23)

8-8: Imports look correct for dataclasses.
No major concerns regarding importing field, fields, Field collectively here.


11-23: Additional typing imports appear appropriate.
These imports align with the introduced type hints. No issues detected.


32-33: Conditional import for type checking is standard best practice.
Prevents runtime overhead by loading ArgumentParser only when needed for type checks.


42-42: Return type annotation for get_items_by_category is consistent with the yielded tuples.
Matches the pattern of (field_name, value) iteration.


52-53: Introduction of TaggedSettings and usage of str|None is fine under Python 3.10+.
No concerns with the new _inner typing.


55-57: Method signature for register_settings is clear.
Storing settings by an optional tag in _inner is straightforward.


67-67: Potentially misleading return type annotation.
The function returns getattr(settings, field_name), which might not always be a Sequence[Any]. Verify that the attribute is guaranteed to be a sequence.

Would you like to confirm usage by searching for all calls to get_field_settings and checking the actual field types?


73-73: Iterator return hint for __iter__ is correct.
Exposes internal settings in _inner.


77-80: Generic type variable TSettingsBase enhances clarity.
The abstract PluginBase now gracefully supports different SettingsBase subclasses.


91-91: Optional return type for settings_cls is appropriate.
Allows for the possibility of no settings.


97-97: Helper method has_settings_cls remains clear and concise.
No issues identified.


106-106: Nested function fmt_default provides a clear approach to default-value formatting.
Implementation looks correct.


132-132: Type annotation for ArgumentParser in register_cli_args is consistent with usage.
No concerns.


137-137: Early return logic when settings_cls is absent is consistent.
No concerns detected.


191-192: Nested get_description function in validate_settings is coherently typed.
Implementation aligns well with the field-based checks.


214-214: Return type annotation for get_settings is accurate.
Appropriately covers both tagged and untagged scenarios.


221-222: Local variable dc for the settings class and the subsequent None check are logical.
No additional concerns.


226-226: Casting a base SettingsBase to the generic type helps satisfy the type system.
No evident issues.


230-232: Nested helper function get_name_and_value coordinates well with field prefixing logic.
No concerns.


236-237: Dictionaries for tagged vs. untagged kwargs are well-defined.
No noticeable issues in reading or maintainability.


251-253: extract_values nested function with typed parameters is straightforward.
No concerns regarding logic or usage.


271-272: Use is and is not for type comparisons instead of == or !=.
This repeats a past comment about line 271. Please update to match best practices:

-elif thefield.type != str and isinstance(thefield.type, type):
+elif thefield.type is not str and isinstance(thefield.type, type):
🧰 Tools
🪛 Ruff (0.8.2)

271-271: Use is and is not for type comparisons, or isinstance() for isinstance checks

(E721)


321-322: Nested function apply_type in _parse_type is suitably typed.
Implementation for optional/unions appears valid.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/snakemake_interface_common/settings.py (2)

100-104: Provide a custom error message for invalid choices.

Relying on KeyError may produce a less informative error. Consider raising a more descriptive exception, for example:

- return cls[choice.replace("-", "_").upper()]
+ try:
+     return cls[choice.replace("-", "_").upper()]
+ except KeyError:
+     raise ValueError(f"Invalid choice: {choice}. Must be one of {cls.choices()}.")

105-118: Consider adding dedicated tests for the new settings enumeration.

Unit tests can verify that parsing choices, skipping members, and generating string representations behave as intended. Let me know if you need help creating such tests or if you’d like me to open a new issue tracking this task.

.github/workflows/test.yml (1)

39-42: Consider uncommenting the Mypy step

Since this PR is specifically focused on fixing mypy complaints, it would be beneficial to enable the type checking step in the workflow to verify that all issues have been resolved.

-      # - name: Mypy
-      #   if: always()
-      #   run: |
-      #     pixi run --environment dev type-check
+      - name: Mypy
+        if: always()
+        run: |
+          pixi run --environment dev type-check
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3d63108 and e5a3b7c.

⛔ Files ignored due to path filters (2)
  • pixi.lock is excluded by !**/*.lock
  • pyproject.toml is excluded by !pyproject.toml
📒 Files selected for processing (14)
  • .gitattributes (1 hunks)
  • .github/workflows/release-please.yml (1 hunks)
  • .github/workflows/test.yml (1 hunks)
  • .gitignore (1 hunks)
  • snakemake_interface_common/settings.py (0 hunks)
  • src/snakemake_interface_common/exceptions.py (4 hunks)
  • src/snakemake_interface_common/logging.py (1 hunks)
  • src/snakemake_interface_common/plugin_registry/__init__.py (5 hunks)
  • src/snakemake_interface_common/plugin_registry/attribute_types.py (1 hunks)
  • src/snakemake_interface_common/plugin_registry/plugin.py (13 hunks)
  • src/snakemake_interface_common/plugin_registry/tests.py (2 hunks)
  • src/snakemake_interface_common/rules.py (1 hunks)
  • src/snakemake_interface_common/settings.py (1 hunks)
  • src/snakemake_interface_common/utils.py (5 hunks)
💤 Files with no reviewable changes (1)
  • snakemake_interface_common/settings.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

  • src/snakemake_interface_common/logging.py
  • src/snakemake_interface_common/settings.py
  • src/snakemake_interface_common/exceptions.py
  • src/snakemake_interface_common/plugin_registry/attribute_types.py
  • src/snakemake_interface_common/utils.py
  • src/snakemake_interface_common/rules.py
  • src/snakemake_interface_common/plugin_registry/tests.py
  • src/snakemake_interface_common/plugin_registry/__init__.py
  • src/snakemake_interface_common/plugin_registry/plugin.py
🪛 Ruff (0.8.2)
src/snakemake_interface_common/plugin_registry/plugin.py

256-256: Use is and is not for type comparisons, or isinstance() for isinstance checks

(E721)

🔇 Additional comments (61)
.gitattributes (1)

1-2: Good practice for lock files.

Defining pixi.lock as a binary file and marking it as generated ensures merge conflicts are avoided and that the file is treated correctly by GitHub’s linguist. This approach is well-aligned with standard practices for lock files.

.gitignore (1)

162-165: Reevaluate ignoring poetry.lock.

It’s generally recommended to commit poetry.lock for reproducible builds, especially for distributed teams. Please verify that ignoring poetry.lock is indeed desired. The additions for ignoring .pixi and *.egg-info are standard and look good.

src/snakemake_interface_common/logging.py (2)

5-9: Effective use of TYPE_CHECKING.

Placing imports under if TYPE_CHECKING: is a clean way to prevent circular dependencies while still providing type hints to static analyzers.


11-13: Correct return type annotation.

Specifying the return type as "logging.Logger" is appropriate to avoid runtime import issues and maintain clarity for mypy. This change looks good.

src/snakemake_interface_common/settings.py (1)

1-99: Comprehensive and well-documented enumeration base class.

The structure and docstrings for SettingsEnumBase are thorough. The approach of handling both internal and external representations is clear, and the helper methods (_parse_choices_into, all, etc.) make the codebase more uniform and maintainable.

src/snakemake_interface_common/plugin_registry/attribute_types.py (1)

23-23: Type annotations properly added to methods

The addition of return type annotations to is_optional, is_class, and into_required methods improves type safety while maintaining functionality. The self-referential type annotation in into_required is correctly implemented.

Also applies to: 27-27, 30-30

.github/workflows/release-please.yml (1)

25-44: Workflow modernization looks good

The transition from Poetry to Pixi for dependency management appears well-implemented. The updated GitHub actions, installation of Pixi with appropriate version specification, and modern PyPI publishing configuration with token-based authentication represent a good modernization of the CI pipeline.

src/snakemake_interface_common/rules.py (1)

7-7:

❓ Verification inconclusive

Type annotations added to abstract methods

The addition of return type annotations to the abstract methods in RuleInterface improves type safety. Note that if snakefile previously returned Path objects, changing to str return type represents an API change that may require updates to dependent code.

Also applies to: 11-11, 15-15


🏁 Script executed:

#!/bin/bash
# Check if there are any uses of Path methods on the result of snakefile calls
rg -p "\.snakefile\(\)" -A 3 -B 3 | rg "\.stem|\.suffix|\.parent|\.resolve|\.absolute"

Length of output: 88


Abstract Method Type Annotations and Potential API Change

The RuleInterface methods now have explicit return type annotations, which improves type safety. Note that changing the snakefile method’s return type from (presumably) a Path to str represents an API decision that could affect dependent code. Although a shell search for chained Path method calls on snakefile did not produce any results, please manually verify that no modules rely on snakefile returning a Path.

  • File: src/snakemake_interface_common/rules.py
    • Line 7: def name(self) -> str: ...
    • Line 11: def lineno(self) -> int: ...
    • Line 15: def snakefile(self) -> str: ...

Please confirm that all dependent code has been updated accordingly.

src/snakemake_interface_common/exceptions.py (4)

8-8: Class attributes properly typed with appropriate imports

Adding explicit type annotations for class attributes lineno, snakefile, and rule with proper Optional types improves code clarity and type checking capabilities.

Also applies to: 18-20


22-22: Method signatures enhanced with type annotations

Type annotations for method parameters and return types are now properly specified, improving type safety. The change from Path to str for the snakefile parameter type is consistent with the similar change in rules.py.

Also applies to: 44-44, 46-46, 66-66


35-37: Proper handling of ExceptionGroup for Python 3.11+ compatibility

The code correctly handles the Python 3.11+ ExceptionGroup with a version check and noqa comment to avoid linting errors on earlier Python versions.


61-61: Functionally equivalent tuple manipulation

The modified args manipulation using tuple conversion and list operations is functionally equivalent to the previous version but uses more modern Python syntax with f-strings.

.github/workflows/test.yml (5)

10-14: Proper permission configuration added

Adding explicit permissions is a security best practice. By defining specific access scopes, you're following the principle of least privilege.


17-17: Job name change appropriately reflects expanded responsibilities

Renaming from "formatting" to "quality-control" better represents the job's full scope, which now includes formatting, linting, and potentially type checking.


21-28: Updated tooling from Poetry to Pixi

The migration from Poetry to Pixi as the package/environment manager is consistently implemented with appropriate version pinning.


30-38: New formatting and linting steps added

The Ruff formatting and linting steps are properly configured with the --check and --diff flags to ensure code quality without automatically modifying files.


50-60: Testing job updates follow the same pattern

The testing job has been consistently updated to use Pixi in the same way as the quality-control job, maintaining a uniform approach across the workflow.

src/snakemake_interface_common/plugin_registry/tests.py (6)

17-17: Improved abstract method signature with proper return type

The method signature now includes a return type annotation and uses the ellipsis syntax on the same line, which is more concise and helps with static type checking.


20-20: Improved abstract method signature with proper return type

The method signature now includes a return type annotation and uses the ellipsis syntax on the same line, which is more concise and helps with static type checking.


23-23: Improved abstract method signature

The method signature now uses the ellipsis syntax on the same line, which is more concise and helps with static type checking.


26-26: Improved abstract method signature

The method signature now uses the ellipsis syntax on the same line, which is more concise and helps with static type checking.


29-29: Improved abstract method signature with proper return type

The method signature now includes a return type annotation and uses the ellipsis syntax on the same line, which is more concise and helps with static type checking.


43-45: Improved assertion message formatting

The assertion message has been reformatted to span multiple lines for better readability while maintaining the same functionality.

src/snakemake_interface_common/utils.py (9)

5-5: Added necessary type annotation imports

Added imports from the typing module to support the type annotations throughout the file, which is essential for mypy type checking.


8-8: Added type annotations to not_iterable function

The function now has proper type annotations, indicating it takes any value and returns a boolean, which improves type safety.


20-20: Added type annotations to clean method

The static method now has proper type annotations, indicating it takes an instance of Any type and a method name as a string, which improves type safety.


23-23: Added type annotations to init method

The method now has proper type annotations, indicating it takes a Callable and returns None, which improves type safety.


28-28: Added type annotations to get method

The method now has proper type annotations, indicating it takes an instance of Any type and an owner that is either a type or None, and returns Any, which improves type safety.


39-54: Enhanced lutime function with type annotations and improved docstring

The function now has proper type annotations and a well-structured docstring with Parameters and Returns sections, improving both type safety and documentation.


69-69: Added type annotations to fmt_time inner function

The inner function now has proper type annotations, indicating it takes a float and returns a string, which improves type safety.


85-85: Added type annotations to lchmod function (first definition)

The function now has proper type annotations, indicating it takes a string path and an integer mode, and returns None, which improves type safety.


90-90: Added type annotations to lchmod function (second definition)

The function now has proper type annotations, indicating it takes a string path and an integer mode, and returns None, which improves type safety.

src/snakemake_interface_common/plugin_registry/__init__.py (11)

10-10: Added typing imports

Added necessary imports from typing module, including TYPE_CHECKING for conditional import of ArgumentParser.


16-18: Added conditional import for ArgumentParser

Using TYPE_CHECKING guard for the ArgumentParser import helps prevent circular imports at runtime while still providing type information for static type checkers.


24-24: Added type annotation for plugins class attribute

Explicitly typing the plugins dictionary improves type safety and helps static type checkers understand the structure of the data.


26-26: Added return type annotation to new method

Adding the return type annotation clarifies that the method returns an instance of PluginRegistryBase, improving type safety.


31-31: Added return type annotation to init method

Adding the return type annotation clarifies that the method returns None, improving type safety.


60-60: Added return type annotation to register_cli_args method

Adding the return type annotation clarifies that the method returns None, improving type safety.


66-66: Added return type annotation to collect_plugins method

Adding the return type annotation clarifies that the method returns None, improving type safety.


84-84: Added return type annotation to register_plugin method

Adding the return type annotation clarifies that the method returns None, improving type safety.


99-99: Added return type annotation to validate_plugin method

Adding the return type annotation clarifies that the method returns None, improving type safety.


127-127: Improved abstract method signature

The abstract method now uses the ellipsis syntax on the same line and includes a return type annotation, making it more concise and type-safe.


135-135: Improved abstract method signature

The abstract method now uses the ellipsis syntax on the same line and includes a return type annotation, making it more concise and type-safe.

src/snakemake_interface_common/plugin_registry/plugin.py (18)

8-8: Appropriate import addition

Good addition of the Field import from dataclasses, which is now used in type annotations throughout the file.


11-11: Well-structured import expansion

The expanded imports from typing properly include all the types used throughout the file, improving code clarity and type safety.


20-21: Good practice for conditional import

Using conditional imports with TYPE_CHECKING is a good practice to avoid circular imports while still providing proper type information to static type checkers.


30-30: Improved return type annotation

The explicit return type annotation for get_items_by_category makes the method's contract clearer and improves type safety.


41-41: Enhanced type safety for dictionary

The updated type hint for _inner correctly allows for str | None as keys, which aligns with the actual usage in the code.


43-45: Added return type for clarity

Adding the None return type to register_settings explicitly indicates that the method doesn't return anything.


55-55: Improved dictionary return type

The updated return type for get_field_settings correctly reflects that keys can be str | None, matching the type of _inner.


61-61: Enhanced iterator typing

The explicit iterator return type for __iter__ improves type safety and makes the method's contract clearer.


68-68: Concise abstract property return types

The abstract properties now use concise return type annotations, which improves readability while maintaining type safety.

Also applies to: 72-72, 76-76


82-82: Added boolean return type

Adding the bool return type to has_settings_cls makes the method's contract clearer.


91-91: Properly typed parameter

Adding the Field type annotation to the fmt_default parameter improves type safety.


117-117: Explicit None return type

Adding the None return type to register_cli_args explicitly indicates that the method doesn't return anything.


176-177: Improved type annotations

The parameter type hint for validate_settings and the nested function get_description now have proper type annotations, enhancing type safety.


199-199: Enhanced return type specificity

The return type for get_settings now correctly specifies either SettingsBase or TaggedSettings as possible return values.


215-215: Clear function signature with tuple return type

Adding type hints to get_name_and_value improves readability and type safety, especially with the tuple return type.


221-222: Properly typed dictionaries

The updated type hints for kwargs_tagged and kwargs_all correctly reflect their structure and usage in the code.


236-238: Well-typed function parameters

Adding type hints to the extract_values function parameters and return type improves code clarity and type safety.


306-307: Comprehensive type annotations for nested functions

Adding type hints to _parse_type and its nested function apply_type improves type safety and code clarity.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
.github/workflows/test.yml (3)

23-29: Review: Install Pixi Step

The Pixi installation step is well configured with the specified version (v0.42.1), environment (dev), and caching enabled. One minor nitpick: verify that the parameter name environments (plural) is intentionally used here, as subsequent command flags use the singular --environment. Consistency here helps avoid potential confusion.


45-47: Review: Collect QC Step

The "Collect QC" step provides a simple echo confirming that all quality control checks passed. For enhanced diagnostics in the future, consider integrating actual output collection from the previous steps.


53-59: Review: Install Pixi in Testing Job

This step duplicates the Pixi installation from the quality-control job, using identical parameters. To reduce repetition and simplify maintenance, consider consolidating this configuration using YAML anchors or a reusable workflow component.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5a3b7c and 58b0a94.

📒 Files selected for processing (1)
  • .github/workflows/test.yml (1 hunks)
🔇 Additional comments (8)
.github/workflows/test.yml (8)

10-15: Review: Permissions Block Setup

The new permissions block correctly grants read access for contents and write access for checks, issues, and pull-requests. This enhances the workflow's ability to interact with PR checks and issues. Please double-check that these permissions are strictly necessary and don’t inadvertently provide broader access than intended.


17-19: Review: Job Naming and Environment Setup

The job has been successfully renamed to "quality-control" to better reflect its expanded role (formatting, linting, type-checking, etc.). The runner environment ("ubuntu-latest") is appropriately set.


21-21: Review: Checkout Action Update

The use of actions/checkout@v4 is a welcome upgrade that ensures improved performance and access to newer GitHub features.


30-34: Review: Ruff Format Check

The "Ruff Format" step is correctly set up with an if: always() condition and executes the formatting check using Pixi. This helps ensure that code style standards are enforced consistently.


35-39: Review: Ruff Lint Step

This step uses Pixi to lint the code with a diff output, which is useful for quickly identifying improvements after formatting. Its configuration with if: always() is appropriate.


40-44: Review: Mypy Type-Check Step

Integrating the mypy step with Pixi (pixi run --environment dev type-check) aligns with the PR objective to resolve type issues. The configuration appears correct.


51-52: Review: Code Checkout in Testing Job

The testing job correctly checks out the repository using actions/checkout@v4, ensuring that the latest code is used during tests.


60-62: Review: Run Tests Command

The test command now leverages Pixi (pixi run --environment dev test --show-capture=all -s -vv), which is consistent with the new dependency setup. Ensure that the flags used (--show-capture=all -s -vv) provide the desired verbosity and output for your test suite.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
.github/workflows/test.yml (1)

44-46: Collect Quality Control (QC) Message:
The "Collect QC" step simply echoes a confirmation message. While it currently serves as a placeholder, consider enhancing this step in the future with more detailed reporting or integration with your CI reporting tools if needed.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 58b0a94 and d801c7d.

📒 Files selected for processing (2)
  • .github/workflows/test.yml (1 hunks)
  • .gitignore (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • .gitignore
🔇 Additional comments (10)
.github/workflows/test.yml (10)

10-15: Enhanced Permissions Configuration:
The addition of an explicit permissions section (with contents: read, checks: write, issues: write, and pull-requests: write) is a positive change. It clearly outlines the minimal rights required for the workflow, aligning with the principle of least privilege.


17-19: Job Renaming & Setup:
Renaming the job from a generic name to quality-control better reflects that this job now encompasses multiple quality assurance tasks (formatting, linting, and type-checking). The use of runs-on: ubuntu-latest is standard and appropriate.


20-22: Checkout Action Update:
Updating the checkout action to actions/checkout@v4 ensures that the workflow benefits from the latest updates and security fixes. This is an excellent move toward keeping dependencies up to date.


23-28: Introducing Pixi Installation:
Replacing the Poetry installation with Pixi via prefix-dev/[email protected] aligns with the repo’s shift toward improved type checking and dependency management. The provided inputs for environments: dev and pixi-version: v0.42.1 are clear and consistent.


29-33: Ruff Format Step Addition:
Adding the "Ruff Format" step using Pixi (pixi run --environment dev format --check) integrates automated formatting into the CI pipeline. The use of if: always() guarantees execution regardless of previous step failures, which is useful for reporting purposes.


34-38: Ruff Linting Integration:
The new "Ruff lint" step ensures that code style issues are flagged early. The use of --diff provides clarity on the corrections needed, supporting a more efficient review process.


39-43: Mypy Type-Check Step:
Running mypy through Pixi (pixi run --environment dev type-check) directly addresses the mypy complaint resolutions. This clear separation of type checking boosts confidence in the type annotations across the repository.


47-51: Testing Job – Checkout Consistency:
The testing job continues to use actions/checkout@v4, ensuring that the codebase is checked out with the same updated version as in the quality-control job. This consistency is beneficial for maintainability.


52-57: Pixi Setup in Testing Job:
The testing job now includes a Pixi installation step with identical environment settings (environments: dev and pixi-version: v0.42.1) as in the quality-control job. This uniformity helps maintain environment consistency across different stages.


58-60: Test Execution with Pixi:
The test step uses Pixi for running tests (pixi run --environment dev test --show-capture=all -s -vv), which aligns with the new dependency management approach. The inclusion of verbose flags (-s -vv and --show-capture=all) is helpful for debugging and ensuring complete output capture during test runs.

@jjjermiah jjjermiah force-pushed the jjjermiah/fix-mypy-basic branch from 5d29e35 to ea66427 Compare March 27, 2025 13:53
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/snakemake_interface_common/plugin_registry/plugin.py (1)

271-271: Use is not instead of != for direct type comparison.
For clarity and alignment with Python style guidelines (E721), compare type objects with is or is not:

-elif thefield.type != str and isinstance(thefield.type, type):
+elif thefield.type is not str and isinstance(thefield.type, type):
🧰 Tools
🪛 Ruff (0.8.2)

271-271: Use is and is not for type comparisons, or isinstance() for isinstance checks

(E721)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d29e35 and ea66427.

⛔ Files ignored due to path filters (1)
  • pyproject.toml is excluded by !pyproject.toml
📒 Files selected for processing (9)
  • .github/workflows/test.yml (1 hunks)
  • src/snakemake_interface_common/exceptions.py (4 hunks)
  • src/snakemake_interface_common/logging.py (1 hunks)
  • src/snakemake_interface_common/plugin_registry/__init__.py (5 hunks)
  • src/snakemake_interface_common/plugin_registry/attribute_types.py (1 hunks)
  • src/snakemake_interface_common/plugin_registry/plugin.py (13 hunks)
  • src/snakemake_interface_common/rules.py (1 hunks)
  • src/snakemake_interface_common/settings.py (1 hunks)
  • src/snakemake_interface_common/utils.py (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • .github/workflows/test.yml
  • src/snakemake_interface_common/rules.py
  • src/snakemake_interface_common/logging.py
  • src/snakemake_interface_common/plugin_registry/attribute_types.py
  • src/snakemake_interface_common/exceptions.py
  • src/snakemake_interface_common/plugin_registry/init.py
  • src/snakemake_interface_common/utils.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

  • src/snakemake_interface_common/settings.py
  • src/snakemake_interface_common/plugin_registry/plugin.py
🧬 Code Definitions (1)
src/snakemake_interface_common/plugin_registry/plugin.py (3)
src/snakemake_interface_common/plugin_registry/__init__.py (1)
  • register_cli_args (64-68)
src/snakemake_interface_common/plugin_registry/tests.py (1)
  • validate_settings (26-26)
src/snakemake_interface_common/rules.py (1)
  • name (7-7)
🪛 Ruff (0.8.2)
src/snakemake_interface_common/plugin_registry/plugin.py

271-271: Use is and is not for type comparisons, or isinstance() for isinstance checks

(E721)

🔇 Additional comments (3)
src/snakemake_interface_common/settings.py (2)

10-64: Comprehensive docstring improvements look good.
The updated docstrings provide clear guidance on usage and highlight each method’s purpose. This enhances readability and maintainability.


90-101: Potential type mismatch in parse_choice return type.
The _parse_choices_into method casts cls.parse_choice(choice) to TSettingsEnumBase, but parse_choice is declared to return "SettingsEnumBase". Additionally, the docstring suggests it returns TSettingsEnumBase. For consistency, consider updating the return annotation to match your usage and docstring:

-    def parse_choice(cls, choice: str) -> "SettingsEnumBase":
+    def parse_choice(cls, choice: str) -> TSettingsEnumBase:
     """Converts a single string choice into an enum member."""
     ...
src/snakemake_interface_common/plugin_registry/plugin.py (1)

321-347: Good handling of optional vs. non-optional types.
This _parse_type logic carefully checks if a type is typing.Optional or a single plain type, correctly raising errors for unsupported unions.

🧰 Tools
🪛 Ruff (0.8.2)

326-332: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

1 participant