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

use declarative metadata #442

Closed
wants to merge 5 commits into from

Conversation

graingert
Copy link
Member

No description provided.

setup.cfg Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
Comment on lines +1 to +9
import pathlib
import re

PACKAGE = "httpx"


def get_version(package=PACKAGE):
version = pathlib.Path(package, "__version__.py").read_text()
return re.search("__version__ = ['\"]([^'\"]+)['\"]", version).group(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this part into httpx/httpx/__version__.py

Copy link
Member Author

Choose a reason for hiding this comment

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

No because you can't import httpx until after setup

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly this one edge-case has me not wanting to merge this. :(

Copy link
Member Author

Choose a reason for hiding this comment

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

setup.py already does not import http.__version__.__version__ because of this limitation. It's not an edge case

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant having this file to accomplish only this one task.

@tomchristie tomchristie self-requested a review October 4, 2019 09:53
setup.py Show resolved Hide resolved
@@ -1,3 +1,50 @@
[metadata]
name = httpx
version = attr: src.get_version
Copy link
Member Author

Choose a reason for hiding this comment

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

Imho this should be a file (httpx/version.txt) and we use importlib_resources in __version__ to grab it

name = httpx
version = attr: src.get_version
url = https://github.com/encode/httpx
license = BSD
Copy link
Member Author

@graingert graingert Oct 4, 2019

Choose a reason for hiding this comment

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

Strictly you don't need license = if there's a trove available

There's no trove classifier for BSD-3-Clause

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
license = BSD
license = BSD-3-Clause

version = attr: src.get_version
url = https://github.com/encode/httpx
license = BSD
description = The next generation HTTP client.
Copy link
Member Author

@graingert graingert Oct 4, 2019

Choose a reason for hiding this comment

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

This is duplicated from httpx.__version__.__description__ because this key doesn't support attr: if people are ok with me adding importlib_resources I'll change it to a file:

setup.cfg Outdated Show resolved Hide resolved
Development Status :: 3 - Alpha
Environment :: Web Environment
Intended Audience :: Developers
License :: OSI Approved :: BSD License
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 license is wrong. This package is licensed under the BSD 3 clause

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
License :: OSI Approved :: BSD License

@graingert
Copy link
Member Author

This also fixes #193

Here it is showing that py.typed ends up in the sdist and whl https://asciinema.org/a/6HXD5BEhcKtetzesXuCY8Fkex

@sethmlarson
Copy link
Contributor

So I don't think we should do this unless we want to move to flit+pyproject.toml (which I'd be a fan of, idk about what others think @florimondmanca, @tomchristie?)

@florimondmanca
Copy link
Member

florimondmanca commented Oct 4, 2019

Although this is a very interesting change (I didn't know setuptools supported setup.cfg-based metadata declaration), I think that's also the exact reason why this should not get in.

All other Encode projects use setup.py to define package metadata. It's a straight-forward, battle-tested approach, and I think changing the way we do things just in HTTPX breaks the principle of least astonishment and tends to increase maintenance overhead.

So, I agree with @sethmlarson and would be in favor of closing this.

I'd be open to see how switching to a pyproject.toml-based tool (flit, poetry?) would look like as well, but honestly I think the current setup works just fine.

@sethmlarson
Copy link
Contributor

Gonna go ahead and close this, thank you for putting this together! Would love if you went through all the setuptools options we're missing like issue tracker, docs, etc.

@sethmlarson sethmlarson closed this Oct 4, 2019
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.

4 participants