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.

Sign up for GitHub

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

Jump to bottom

Use importlib instead of deprecated pkg_resources. Reorganize setup #188

Merged
merged 6 commits into from
Dec 1, 2023
Merged

Use importlib instead of deprecated pkg_resources. Reorganize setup #188

merged 6 commits into from
Dec 1, 2023

Conversation

mcara
Copy link
Member

@mcara mcara commented Nov 3, 2023 •
edited
Loading

@mcara mcara added this to the Better Awsomeness milestone Nov 3, 2023
@mcara mcara requested a review from zacharyburnett November 3, 2023 00:20
@mcara mcara self-assigned this Nov 3, 2023
@mcara mcara requested a review from a team as a code owner November 3, 2023 00:20
@codecov Codecov
Copy link

codecov bot commented Nov 3, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (11d28e9) 94.38% compared to head (26fbcda) 94.38%.

Files Patch % Lines
tweakwcs/__init__.py 50.00% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #188   +/-   ##
=======================================
  Coverage   94.38%   94.38%           
=======================================
  Files          11       11           
  Lines        1941     1941           
  Branches      295      350   +55     
=======================================
  Hits         1832     1832           
  Misses         91       91           
  Partials       18       18

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

@mcara
Copy link
Member Author

mcara commented Nov 3, 2023

@zacharyburnett Should I split this PR into two? This one cannot be merged (tests are failing in Python 3.12) until a new gwcs version is released.

@zacharyburnett
Copy link
Collaborator

@zacharyburnett Should I split this PR into two? This one cannot be merged (tests are failing in Python 3.12) until a new gwcs version is released.

I don't think that's necessary; can we wait for the new gwcs version, and then revisit this?

@mcara mcara mentioned this pull request Nov 29, 2023
Merged
@pllim
Copy link
Contributor

pllim commented Nov 29, 2023

What is the difference between Azure "Publish" vs Actions "Build source and wheel distribution"? Do you really need both?

pllim
pllim reviewed Nov 30, 2023
View reviewed changes
tweakwcs/__init__.py Outdated
# package is not installed
__version__ = 'UNKNOWN'

from importlib.metadata import version
Copy link
Contributor

@pllim pllim Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think you need importlib at all. Try this:

try:
    from ._version import version as __version__
except ImportError:
    __version__ = ''

Copy link
Collaborator

@zacharyburnett zacharyburnett Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would also include ModuleNotFoundError:

try:
    from ._version import version as __version__
except (ImportError, ModuleNotFoundError):
    __version__ = ''

so that if this is copy-pasted elsewhere it will continue to work

Copy link
Contributor

Choose a reason for hiding this comment

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

No one should blindly copy-paste stuff but point taken and it doesn't hurt.

pllim
pllim reviewed Nov 30, 2023
View reviewed changes
azure-templates.yml Outdated
Copy link
Contributor

@pllim pllim Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If you advertise that you support python >= 3.7, then you should test also 3.7 and 3.8. But really 3.7 is already EOL, so I think it is okay to do python_requires='>=3.8'. Still you should keep the 3.8 job.

pllim
pllim reviewed Nov 30, 2023
View reviewed changes
setup.py Outdated
@@ -110,7 +110,7 @@ def get_transforms_data():
'Topic :: Software Development :: Libraries :: Python Modules',
'Development Status :: 3 - Alpha',
],
python_requires='>=3.5',
python_requires='>=3.7',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
python_requires='>=3.7',
python_requires='>=3.8',

@mcara
Copy link
Member Author

mcara commented Nov 30, 2023

According to mathematical induction.... if it works for 3.10 and it works for 3.9, it will work for 3.8. ... I think

pllim
pllim reviewed Nov 30, 2023
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
pllim
pllim reviewed Nov 30, 2023
View reviewed changes
pyproject.toml
'stsci.stimage',
'stsci.imagestats',
'spherical_geometry>=1.2.20',
'packaging>=19.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you need this as runtime dependency?

Suggested change
'packaging>=19.0',

pllim
pllim reviewed Nov 30, 2023
View reviewed changes
pyproject.toml
test = [
"pytest",
"pytest-cov",
"coverage",
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need this.

Suggested change
"coverage",

pllim
pllim reviewed Nov 30, 2023
View reviewed changes
pyproject.toml
'numpydoc',
'sphinx',
'sphinx-automodapi',
'sphinx-rtd-theme',
Copy link
Contributor

Choose a reason for hiding this comment

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

You are not using this.

Suggested change
'sphinx-rtd-theme',

pllim
pllim reviewed Nov 30, 2023
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
pllim
pllim reviewed Nov 30, 2023
View reviewed changes
pyproject.toml
Comment on lines +81 to +85
'README.rst',
'LICENSE.txt',
'CHANGELOG.rst',
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless your code actually parses these files, they are not package data.

Suggested change
'README.rst',
'LICENSE.txt',
'CHANGELOG.rst',

Copy link
Member Author

Choose a reason for hiding this comment

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

yes they are if I want them to be. One does not need to parse them in the code. They can be parsed with eyes.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can include them in MANIFEST without making them actual package data for eye-feasting.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's OK. astropy adds CITATION: https://github.com/astropy/astropy/blob/164a67784e669b30c859db904668e04b9f6e1bfa/pyproject.toml#L145 No one cares.

Copy link
Contributor

Choose a reason for hiding this comment

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

The CITATION one is some copy-and-paste artifact from astropy maybe? astropy does have a Python API for citation, so it is included there on purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

astropy does have a Python API for citation

I must say: WOW!

Copy link
Contributor

Choose a reason for hiding this comment

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

https://www.astropy.org/acknowledging.html

import astropy
astropy.__citation__

pllim
pllim reviewed Nov 30, 2023
View reviewed changes
pyproject.toml Outdated
Comment on lines 84 to 90
'*.fits',
'*.txt',
'*.inc',
'*.cfg',
'*.csv',
'*.yaml',
'*.json',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be more specific on these ones. You don't want to bundle accidental data.

@pllim
Copy link
Contributor

pllim commented Nov 30, 2023

if it works for 3.10 and it works for 3.9, it will work for 3.8

That is not a given. For example, you might be locked to an older numpy in Python 3.8, and then some code might not work in that combo if you stop testing it.

@mcara
Copy link
Member Author

mcara commented Nov 30, 2023 •
edited
Loading

if it works for 3.10 and it works for 3.9, it will work for 3.8

That is not a given. For example, you might be locked to an older numpy in Python 3.8, and then some code might not work in that combo if you stop testing it.

  1. [For the record] I was kidding about mathematical induction;
  2. If, let's say 3.7 is no longer under development and the numpy version which runs with 3.7 is unlikely to change, how can the code suddenly stop working? The only way would be to use some cutting edge features from Python 3.8 that are not available in 3.7. I think that is also very very unlikely to happen.

@pllim
Copy link
Contributor

pllim commented Nov 30, 2023

how can the code suddenly stop working? The only way would be to use some cutting edge features from Python 3.8 that are not available in 3.7

Yes, exactly that. New code that is somehow not compatible to the oldest version(s) you claim to support. Even using a new keyword in some popular numpy function and so on counts. Or something as simple as things like f-string.

@mcara mcara changed the title Use importlib instead of deprecated pkg_resources Use importlib instead of deprecated pkg_resources. Reorganize setup Nov 30, 2023
@mcara mcara merged commit b0194d0 into spacetelescope:master Dec 1, 2023
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zacharyburnett zacharyburnett zacharyburnett approved these changes

@pllim pllim pllim left review comments

Assignees

@mcara mcara

Labels
Maintenance no changelog entry needed
Projects
None yet
Milestone
Better Awsomeness
Development

Successfully merging this pull request may close these issues.
3 participants
@mcara @zacharyburnett @pllim