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

Remove setup.cfg in favor of pyproject.toml #58

Merged
merged 7 commits into from
Dec 9, 2024
Merged

Remove setup.cfg in favor of pyproject.toml #58

merged 7 commits into from
Dec 9, 2024

Conversation

AdamRJensen
Copy link
Member

Closes #57

@AdamRJensen
Copy link
Member Author

AdamRJensen commented Oct 15, 2024 •
edited
Loading

  • Figure out how to infer the package version (or whether to retain the hardcoded value). link

@kandersolar
Copy link
Member

pvlib-python uses setuptools_scm

@AdamRJensen
Copy link
Member Author

With the new modification, the dynamic versioning seems to work.

I verified this by pip installing the package from my local development repository and checking the version
image

kandersolar
kandersolar reviewed Oct 22, 2024
View reviewed changes
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

The new wheel file includes data and test folders, increasing file size to 1.8 MB versus 13 kB for the latest PyPI wheel.

pyproject.toml Outdated
"Intended Audience :: Science/Research",
]
dependencies = [
'importlib-metadata; python_version<"3.10"',
Copy link
Member

Choose a reason for hiding this comment

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

Where did importlib-metadata come from? Do we need it? I thought it was only needed for python<3.8.

pyproject.toml Outdated
"setuptools>=42",
"wheel"
]
requires = ["setuptools", "setuptools-scm"]
Copy link
Member

Choose a reason for hiding this comment

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

We should require >=61 for setuptools, since that's when it started supporting pyproject.toml/PEP 621.
https://setuptools.pypa.io/en/stable/history.html#v61-0-0

pyproject.toml Outdated
Comment on lines 9 to 12
authors = [
{name = "Adam R. Jensen", email = "adam-r-j@hotmail.com"},
{name = "Kevin S. Anderson"},
]
Copy link
Member

Choose a reason for hiding this comment

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

This results in the wheel's METADATA file including this, which doesn't seem quite right to me:

Author: Kevin S. Anderson
Author-email: "Adam R. Jensen" <adam-r-j@hotmail.com>

The v0.2.5 wheel on PyPI has this, which seems better to me:

Author: 'Adam R. Jensen, Kevin Anderson'
Author-email: adam-r-j@hotmail.com

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems like a strange behavior, see explanation below from this link. I've made my email in a separate email entry now.

If only name is provided, the value goes in Author or Maintainer as appropriate.

If only email is provided, the value goes in Author-email or Maintainer-email as appropriate.

If both email and name are provided, the value goes in Author-email or Maintainer-email as appropriate, with the format {name} <{email}>.

Multiple values should be separated by commas.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the twoaxistracking.egg-info/PKG-INFO file this now lists as:

Author: Adam R. Jensen, Kevin S. Anderson
Author-email: adam-r-j@hotmail.com

@kandersolar kandersolar mentioned this pull request Oct 23, 2024
Open
@AdamRJensen
Copy link
Member Author

@kandersolar I think this is done now.

When I locally install the package using pip install . the version is listed as 0.2.6. I'm uncertain about how it infers this number? The version is indeed correct, but only because we're going for a patch release.

@kandersolar
Copy link
Member

It will switch to 0.3.0 whenever we tag that version. See https://setuptools-scm.readthedocs.io/en/latest/usage/#default-versioning-scheme

I want to take another look at the metadata in the distribution files. I'll do that today.

kandersolar
kandersolar reviewed Dec 9, 2024
View reviewed changes
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Looks OK to me, with one possible issue: docs, twoaxistracking/data, and twoaxistracking/tests are now included in the dist files, increasing the wheel size from 12.5 kB to 2.8 MB. Should they continue to be excluded?

@AdamRJensen
Copy link
Member Author

AdamRJensen commented Dec 9, 2024 •
edited
Loading

Looks OK to me, with one possible issue: docs, twoaxistracking/data, and twoaxistracking/tests are now included in the dist files, increasing the wheel size from 12.5 kB to 2.8 MB. Should they continue to be excluded?

Ahh yes, I'll address that ASAP

Update: this was caused by incorrect formatting of the exclude parameter

kandersolar
kandersolar reviewed Dec 9, 2024
View reviewed changes
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

The below change gets it working for me:

image

(I think leaving tests/data/docs in the sdist is appropriate)

pyproject.toml Outdated
Comment on lines 10 to 14
[tool.setuptools.packages.find]
exclude = [
"tests*",
"data*",
]
Copy link
Member

@kandersolar kandersolar Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
[tool.setuptools.packages.find]
exclude = [
"tests*",
"data*",
]
[tool.setuptools]
include-package-data = false
[tool.setuptools.packages.find]
exclude = [
"twoaxistracking.tests*",
"twoaxistracking.data*",
"docs*",
]

Copy link
Member

Choose a reason for hiding this comment

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

Whoops, exclude data* as well

@AdamRJensen AdamRJensen merged commit eee16f8 into main Dec 9, 2024
17 checks passed
@AdamRJensen AdamRJensen deleted the switch_to_pyproejct_toml branch December 9, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kandersolar kandersolar kandersolar left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transition from setup.cfg to pyproject.toml
2 participants
@AdamRJensen @kandersolar