Skip to content
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

Fix type problems found by latest mypy #75

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

TedBrookings
Copy link
Contributor

@TedBrookings TedBrookings commented Nov 22, 2023

@TedBrookings TedBrookings force-pushed the feature/support-updated-mypy branch 2 times, most recently from e789c01 to 63c109e Compare November 22, 2023 16:01
@TedBrookings TedBrookings requested a review from nh13 November 22, 2023 17:42
Copy link
Member

@nh13 nh13 left a comment

Choose a reason for hiding this comment

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

I see a few regressions to discuss, happy to chat through live.

fgpyo/util/inspect.py Show resolved Hide resolved
return functools.partial(
lambda s: origin_type(
lambda s: list(
Copy link
Member

Choose a reason for hiding this comment

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

I suppose I had thought I would combine the returns in list and set, but that wouldn't be readable. Thank-you for making this clearer!

fgpyo/util/metric.py Show resolved Hide resolved
@@ -226,7 +226,7 @@ def parse(cls, fields: List[str]) -> Any:
return inspect.attr_from(cls=cls, kwargs=dict(zip(header, fields)), parsers=parsers)

@classmethod
def write(cls, path: Path, *values: MetricType) -> None:
def write(cls, path: Path, *values: "Metric") -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Why loose this type hint? It only writes metrics of this type, so should it not be MetricType?

Copy link
Member

Choose a reason for hiding this comment

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

Add the bound on MetricType to have Metric

@@ -9,20 +10,19 @@
from typing import TypeVar
from typing import Union

try:
# `get_origin_type` is a method that gets the outer type (ex list in a List[str])
# `get_arg_types` is a method that gets the inner type (ex str in a List[str])
Copy link
Member

Choose a reason for hiding this comment

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

thank-you for adding this explanation, I forget every time.

fgpyo/util/types.py Show resolved Hide resolved
fgpyo/util/types.py Show resolved Hide resolved
Copy link
Member

@nh13 nh13 left a comment

Choose a reason for hiding this comment

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

LGTM based on in-person review

Closes #73
* Move to current latest version of pytest to avoid deprecation warning
* Move to current latest version of mypy
* Correct multiple problems with type and import flagged by newer mypy
@TedBrookings TedBrookings force-pushed the feature/support-updated-mypy branch from 63c109e to 62f2583 Compare November 22, 2023 20:08
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (ca4a572) 93.01% compared to head (62f2583) 92.88%.

Files Patch % Lines
fgpyo/util/inspect.py 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
- Coverage   93.01%   92.88%   -0.14%     
==========================================
  Files          30       30              
  Lines        3119     3118       -1     
  Branches      579      581       +2     
==========================================
- Hits         2901     2896       -5     
- Misses        145      148       +3     
- Partials       73       74       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TedBrookings TedBrookings merged commit b8ac915 into main Nov 22, 2023
4 of 6 checks 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.

Newer versions of mypy reject multiple lines of fgpyo
2 participants