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

NEP29 Compliance #85

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

NEP29 Compliance #85

wants to merge 4 commits into from

Conversation

bjlittle
Copy link
Contributor

@bjlittle bjlittle commented Sep 26, 2023

This PR adopts the NEP29 Drop Schedule for minimum supported versions of python and numpy.

When working on #84 I noticed that the PyPI versions were wrong for pytest-pyvista. Turns out, incorrect syntax (probably migrated from setup.py or setup.cfg) was used to specify the required python version for the package; minor fix sorted this.

Went the extra mile and also configured the validate-pyproject pre-commit hook to automate continued confidence in the pyproject.toml.

Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

Thanks! I wondered the same thing. About adding validate-pyproject, also need to ask the @MatthewFlamm if it would be useful to add as it would increase run time. If it is adopted here, please consider sending an additional PR to pyvista/pyvista too.

@bjlittle
Copy link
Contributor Author

Thanks! I wondered the same thing. About adding validate-pyproject, also need to ask the @MatthewFlamm if it would be useful to add as it would increase run time. If it is adopted here, please consider sending an additional PR to pyvista/pyvista too.

@tkoyama010 Cool, thanks.

Although, it seems to be a relatively snappy pre-commit hook. I'm not seeing much difference in runtime or resources being used.

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2023

Codecov Report

Merging #85 (5c26917) into main (42f771b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #85   +/-   ##
=======================================
  Coverage   92.13%   92.13%           
=======================================
  Files           2        2           
  Lines          89       89           
=======================================
  Hits           82       82           
  Misses          7        7           

@MatthewFlamm
Copy link
Contributor

Since this package is required for testing pyvista, we can't drop python 3.8 support without dropping it in PyVista. We should probably move the discussion there.

@tkoyama010
Copy link
Member

Oh, I see. I totally forgot this project was used in PyVista😅.

@tkoyama010
Copy link
Member

I started pyvista/pyvista#4960 . Let's discuss it there!

Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

Keep this while we discuss it.

@bjlittle
Copy link
Contributor Author

@MatthewFlamm and @tkoyama010

Whilst we await a decision on this one, I'm going to put the pull-request into draft to ensure it's not merged 👍

@bjlittle bjlittle marked this pull request as draft September 28, 2023 17: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.

4 participants