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 python 3.9 #2526

Merged
merged 1 commit into from Mar 27, 2024
Merged

Drop python 3.9 #2526

merged 1 commit into from Mar 27, 2024

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Mar 25, 2024

The drop schedule according to NEP 29 says to drop 3.9 in the first week of April and the newer SPEC https://scientific-python.org/specs/spec-0000/ already dropped it.

So I think we should follow and for 0.21, also drop 3.9.

I used ruff --select=UP to check what can be changed and opted for now not converting every .format to an f-string and also didn't remove explicit encoding='utf-8' arguments as "explicit is better than implicit":

ruff --select=UP --ignore=UP032,UP012

This comment has been minimized.

@maxnoe maxnoe force-pushed the drop_py39 branch 2 times, most recently from 5b60f75 to f2133c6 Compare March 25, 2024 16:11
@maxnoe
Copy link
Member Author

maxnoe commented Mar 25, 2024

Checking the uncovered lines, it is all error conditions which we cannot really check in the tests reasonably.

I deactivated the hard "wait=True" which makes the whole CI fail in case of sonarqube not being happy.

This comment has been minimized.

1 similar comment

This comment has been minimized.

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

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

Project coverage is 92.59%. Comparing base (01f4a99) to head (a61035d).

Files Patch % Lines
src/ctapipe/instrument/subarray.py 75.00% 3 Missing ⚠️
src/ctapipe/tools/dump_instrument.py 0.00% 3 Missing ⚠️
src/ctapipe/reco/sklearn.py 60.00% 2 Missing ⚠️
src/ctapipe/instrument/camera/geometry.py 0.00% 1 Missing ⚠️
src/ctapipe/instrument/camera/readout.py 0.00% 1 Missing ⚠️
src/ctapipe/io/astropy_helpers.py 50.00% 1 Missing ⚠️
src/ctapipe/io/metadata.py 0.00% 1 Missing ⚠️
src/ctapipe/io/simteleventsource.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2526   +/-   ##
=======================================
  Coverage   92.59%   92.59%           
=======================================
  Files         233      232    -1     
  Lines       20056    20034   -22     
=======================================
- Hits        18571    18551   -20     
+ Misses       1485     1483    -2     

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

This comment has been minimized.

This comment has been minimized.

@maxnoe maxnoe requested review from kosack and Tobychev March 26, 2024 09:09
@@ -272,7 +271,7 @@ def load_config_file(self, path: Union[str, pathlib.Path]) -> None:

if path.suffix in [".yaml", ".yml"]:
# do our own YAML loading
with open(path, "r") as infile:
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a bit scary, prefer the more explicit version.

Copy link
Member Author

Choose a reason for hiding this comment

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

"r" is the default and python won't ever change that, it would break everyone

src/ctapipe/image/extractor.py Outdated Show resolved Hide resolved
src/ctapipe/tools/tests/test_train.py Show resolved Hide resolved
@@ -104,12 +104,12 @@ def test_signal_fraction(tmp_path, gamma_train_clf, proton_train_clf):
],
)

with open(log_file, "r") as f:
with open(log_file) as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer it when the open is explicit about being a read.

kosack
kosack previously approved these changes Mar 26, 2024

This comment has been minimized.

1 similar comment
Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 81.20% Coverage (92.00% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

@maxnoe maxnoe requested review from kosack and Tobychev March 26, 2024 16:48
@maxnoe maxnoe merged commit f8d1049 into main Mar 27, 2024
13 of 15 checks passed
@maxnoe maxnoe deleted the drop_py39 branch March 27, 2024 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants