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

Remove Python <=3.6 support #1061

Merged
merged 2 commits into from May 14, 2022
Merged

Conversation

pelson
Copy link
Contributor

@pelson pelson commented May 4, 2022

I'm not sure of the schedule, but NEP29 gives some indication that Python 3.6 is becoming legacy (and no longer receives security support patches as per https://endoflife.date/python). At the same time, the CI pipelines now only test Python 3.7+ (upto Python 3.10) - a recent update reasonably (IMO) chose not to put significant effort into testing Python 3.6 in CI, therefore there is no guarantee that the next version of JPype will actually support it.

Given all of the above, I went through the codebase to try to find places where we can remove old bits of code, and update the minimum dependency of JPype to Python 3.7. I am comfortable if this PR is rejected or put on hold (so long as there is a condition for un-holding it put in place).

@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #1061 (b93eb28) into master (61c98a1) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #1061      +/-   ##
==========================================
- Coverage   88.65%   88.65%   -0.01%     
==========================================
  Files         112      112              
  Lines       10236    10232       -4     
  Branches     4012     4012              
==========================================
- Hits         9075     9071       -4     
  Misses        702      702              
  Partials      459      459              
Impacted Files Coverage Δ
jpype/_core.py 95.39% <ø> (-0.09%) ⬇️
jpype/_pykeywords.py 100.00% <ø> (ø)
jpype/imports.py 78.78% <ø> (ø)
jpype/pickle.py 88.09% <ø> (ø)
jpype/protocol.py 98.71% <ø> (ø)
native/python/pyjp_class.cpp 85.73% <ø> (ø)
native/python/pyjp_module.cpp 83.24% <ø> (ø)
native/python/pyjp_array.cpp 78.63% <50.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61c98a1...b93eb28. Read the comment docs.

Copy link
Contributor

@Thrameos Thrameos left a comment

Choose a reason for hiding this comment

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

LGTM. Excellent work.

@Thrameos
Copy link
Contributor

Thrameos commented May 4, 2022

I think we should include this in the next release. Though the decision should go to @marscher .

Thanks for taking care of this. I am unfortunately pulling a "no lifer" on a work project creating a 30k line code from scratch to act as a realtime spectral gamma server for an augmented reality trainer. (>65 hr/wk) The good news is that I beat on jpype paths hourly (largely without hitting anything that requires patching, though the inability to pass single abstract methods (SAM) to lmfit/scipy optimize) is certainly at the annoyance level).

@pelson
Copy link
Contributor Author

pelson commented May 5, 2022

Thanks for the feedback and approval. I found a few more PY_VERSION_HEX cases, so added a commit.

@ap--
Copy link

ap-- commented May 5, 2022

Hi everyone,

Together with this PR it might be worth running pyupgrade --py37-plus over the python part of the codebase to upgrade older style syntax in an automated way.

Cheers,
Andreas 😃

@Thrameos
Copy link
Contributor

Minor issue. The release scripts got missed. These are vital as they decide which artifacts will get built. Thus adding or removing support has to update those hidden scripts in .azure

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.

None yet

4 participants