Skip to content

🔧 MAINTAIN: Switch to declarative metadata #139

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

Merged
merged 4 commits into from
Apr 1, 2021

Conversation

hukkin
Copy link
Contributor

@hukkin hukkin commented Mar 5, 2021

I was testing out some tooling (setup-py-upgrade and setup-cfg-fmt) and came up with this auto-generated PR.

Declarative metadata has some advantages:

  • Can be read by tooling without the need to execute arbitrary Python code
  • Is easy for tools like setup-cfg-fmt to modify. Brings us automatic version classifiers, ordering, etc.
  • No need for some boilerplate like the get_version function.

Maybe we want to keep metadata in setup.py for consistency with other projects though?

I kept a dummy setup.py as pip doesn't yet support editable installs for projects without one.

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #139 (7efdae9) into master (36c386d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #139   +/-   ##
=======================================
  Coverage   96.32%   96.32%           
=======================================
  Files          72       72           
  Lines        3266     3266           
=======================================
  Hits         3146     3146           
  Misses        120      120           
Flag Coverage Δ
pytests 96.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 36c386d...7efdae9. Read the comment docs.

@chrisjsewell
Copy link
Member

rather than installing setuptools, should it be defined in a pyproject.toml, e.g. https://github.com/aiidateam/aiida-core/blob/623d0d8b8b3f11e4f25a0d7d60061dccdba7150a/pyproject.toml#L1

this is the thing I don't like about this "new" setuptools system; you have to replace the single setup.py with three files to specify something I think should all be in pyproject.toml (https://discuss.python.org/t/where-to-get-started-with-pyproject-toml/4906/6)

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

see comment

@hukkin
Copy link
Contributor Author

hukkin commented Mar 13, 2021

rather than installing setuptools, should it be defined in a pyproject.toml, e.g. https://github.com/aiidateam/aiida-core/blob/623d0d8b8b3f11e4f25a0d7d60061dccdba7150a/pyproject.toml#L1

So firstly, setuptools was already installed. The change made here merely updates it similar to what we do with pip because the version that was installed was affected by this setuptools issue which made the build fail when using the setup.cfg mechanism for reading __version__ from Python source. An up-to-date setuptools has this issue fixed.

Adding a pyproject.toml (thus activating PEP 517 build) and restricting setuptools version there would work as well yeah. This comes with the downside that we get yet another file as you mentioned 🤦 😄

this is the thing I don't like about this "new" setuptools system; you have to replace the single setup.py with three files to specify something I think should all be in pyproject.toml (https://discuss.python.org/t/where-to-get-started-with-pyproject-toml/4906/6)

Yeah its pretty annoying right now. With PEP 621 accepted I guess setuptools will soon support metadata in pyproject.toml but we're not there yet. Meanwhile we have a few options:

  • dont change anything
  • merge this PR (declarative metadata)
    • with or without pyproject.toml (i.e. with or without PEP517 builds)
    • with or without the empty setup.py (i.e. with or without pip install -e . support)
  • dump setuptools and use another PEP517/pyproject.toml native build tool (flit, poetry)

@chrisjsewell
Copy link
Member

Oh definitely sticking with setuptools, for editable install support, and because it is more well known for other contributors to work with.
Bit on the fence about the change, but if it is done at some stage I would certainly add the pyproject.toml build dependencies

@hukkin
Copy link
Contributor Author

hukkin commented Mar 17, 2021

Oh definitely sticking with setuptools

👍

for editable install support, and because it is more well known for other contributors to work with.

The other two tools have editable installs, so its not that bad, just not via pip.

Bit on the fence about the change, but if it is done at some stage I would certainly add the pyproject.toml build dependencies

Added the pyproject.toml

@hukkin hukkin force-pushed the declarative-metadata branch from d5949fa to ff2699c Compare March 17, 2021 15:34
Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

Both mypy and flake8 configuration can be moved to setup.cfg, which might be desirable to reduce the number of files:

@@ -1,73 +1,3 @@
# from importlib import import_module
Copy link
Member

Choose a reason for hiding this comment

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

add a comment that this is required only for editable builds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@hukkin hukkin force-pushed the declarative-metadata branch from ff2699c to 7efdae9 Compare March 31, 2021 09:23
@hukkin
Copy link
Contributor Author

hukkin commented Mar 31, 2021

Moved mypy and flake8 conf to setup.cfg

@chrisjsewell chrisjsewell merged commit b1a74b4 into executablebooks:master Apr 1, 2021
@hukkin hukkin deleted the declarative-metadata branch April 1, 2021 08:27
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.

3 participants