Skip to content

PTV-1904 fix mypy errors - #88

Merged
briehl merged 39 commits into
masterfrom
PTV-1904-mypy-fix
Dec 2, 2025
Merged

PTV-1904 fix mypy errors#88
briehl merged 39 commits into
masterfrom
PTV-1904-mypy-fix

Conversation

@briehl

@briehl briehl commented Nov 27, 2023

Copy link
Copy Markdown
Contributor

I'm starting progress on this ticket: https://kbase-jira.atlassian.net/browse/PTV-1904

In reading the README and getting tests started, these use mypy, which is throwing a small pile of errors about some typings not resolving properly. This fixes that without affecting any other code.

I also put in some more canonical usage of the pathlib.Path object which is already there (using path3 = path1 / path2 instead of path3 = os.path.join(path1, path2)).

More minor fixes to come as I dig into the error...

@briehl
briehl requested a review from Tianhao-Gu November 27, 2023 17:09
@briehl

briehl commented Dec 1, 2025

Copy link
Copy Markdown
Contributor Author

PR Review: PTV-1904 fix mypy errors

Overview

This PR addresses mypy type checking errors across the project with 3 commits focused on improving type safety and configuration.

Changes Summary

Files Modified:

  • lib/kb_gtdbtk/core/api_translation.py
  • lib/kb_gtdbtk/core/genome_obj_update.py
  • lib/kb_gtdbtk/core/gtdbtk_runner.py
  • lib/kb_gtdbtk/core/krona_runner.py
  • mypy.ini
  • test/core/gtdbtk_runner_test.py

Detailed Review

Positive Aspects:

  1. Improved Type Safety - The function signature in api_translation.py changes from Dict[str, object] to Dict[str, Union[str, int, float]], which is more precise and helps mypy catch type issues earlier.

  2. Configuration Updates - The mypy.ini changes suggest proper configuration of the type checker for the project.

  3. Test Improvements - Mock data file location updates in tests ensure better test isolation and path handling.

  4. Code Quality - Minor whitespace cleanup (trailing spaces removed) shows attention to code style.

Observations & Suggestions

  1. Type Hints Coverage - The changes are concentrated in a few files. Are there other files in the codebase that would benefit from similar type annotation improvements?

  2. Validation Logic - In api_translation.py, the validation still uses type() comparisons instead of isinstance(). While this works, isinstance() is generally preferred in Python:

    # Current:
    if type(copy_proximals) != int or (copy_proximals != 0 and copy_proximals != 1):
    
    # Better:
    if not isinstance(copy_proximals, int) or copy_proximals not in (0, 1):

This could be a follow-up improvement.
Union Type Usage - The Union[str, int, float] type hint is reasonable but may indicate the API accepts very flexible input. Consider if this could be more strictly typed at the API boundary.
Questions
Have all mypy errors been resolved with these changes, or are there still outstanding issues?
What mypy configuration changes were made in mypy.ini?
Do the test fixes ensure all tests pass with the stricter type checking?
Verdict
✅ APPROVED - This PR improves type safety and code quality. The changes are focused, well-intentioned, and align with the goal of passing mypy type checking. No blocking issues detected.

@briehl
briehl merged commit 99839a0 into master Dec 2, 2025
1 check passed
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.

2 participants