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

Drop support for python 3.7 #98

Merged
merged 18 commits into from
Feb 23, 2024
Merged

Drop support for python 3.7 #98

merged 18 commits into from
Feb 23, 2024

Conversation

jdidion
Copy link
Contributor

@jdidion jdidion commented Feb 22, 2024

No description provided.

Copy link

codecov bot commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 93.81%. Comparing base (dbb600a) to head (7447aff).

Files Patch % Lines
fgpyo/util/inspect.py 94.44% 0 Missing and 1 partial ⚠️
fgpyo/util/types.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
+ Coverage   93.68%   93.81%   +0.12%     
==========================================
  Files          32       33       +1     
  Lines        3374     3363      -11     
  Branches      622      617       -5     
==========================================
- Hits         3161     3155       -6     
+ Misses        140      137       -3     
+ Partials       73       71       -2     

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

@jdidion jdidion force-pushed the 97/jdidion/drop-py37 branch from e78cdd9 to cdffe8a Compare February 22, 2024 17:46
Copy link
Contributor

@TedBrookings TedBrookings left a comment

Choose a reason for hiding this comment

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

There is one change I don't like (style, not function). Otherwise looks good to me.

Comment on lines 84 to 88
else random_generator.uniform(0, 1)
if value_type == VcfFieldType.FLOAT
else random_generator.choice(["Up", "Down"])
else (
random_generator.uniform(0, 1)
if value_type == VcfFieldType.FLOAT
else random_generator.choice(["Up", "Down"])
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the need for this change. The original seems more pythonic to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was made by black after updating to a newer version.

FWIW I find the original more difficult to parse. I'm a fan of using parens to group statements (even when not strictly necessary) to improve readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try to revert the change and see if black complains.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like a series of v1 if c1 else v2 if c2 else c3 is pretty typical for python. But the version black made isn't some unreadable abomination. If you don't care either way I'd say try to revert it. If black doesn't let you, that's that. I approved.

Copy link
Contributor Author

@jdidion jdidion Feb 23, 2024

Choose a reason for hiding this comment

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

Looks like the CI checks pass after reverting.

@jdidion
Copy link
Contributor Author

jdidion commented Feb 23, 2024

I'm not sure why the codecov check is failing. I deleted lots of code that was not covered by the tests, so I would have expected coverage to go up not down.

@TedBrookings
Copy link
Contributor

I suspect it's some crazy math to do with the fraction of lines in the change that are tested? In any case, the 2nd codecov test (overall project coverage) increased. I think you've done your duty and should merge.

@jdidion
Copy link
Contributor Author

jdidion commented Feb 23, 2024

Awesome thanks!

@jdidion jdidion merged commit 111e15b into main Feb 23, 2024
5 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.

2 participants