-
Notifications
You must be signed in to change notification settings - Fork 145
feat: use hatchling, drop setuptools #1323
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
base: master
Are you sure you want to change the base?
Conversation
f7a43c1
to
27cb9ad
Compare
0d9175a
to
e7d0b60
Compare
4eed882
to
2063c20
Compare
3785324
to
8a314c4
Compare
8a314c4
to
0294825
Compare
7a2f2b1
to
28ab5b2
Compare
a5a9aca
to
b4b12ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! I spent some time looking into hatchling + versioningit, but haven't fully reviewed everything just yet, so here are some preliminary comments:
dd74f06
to
2db92a2
Compare
] | ||
|
||
|
||
[tool.check-manifest] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We obviously don't have a MANIFEST.in
anymore, but could it be worth considering finding a replacement for check-manifest somehow, that at least checks the sdist against a preset list of files we expect in there?
pyproject.toml
Outdated
include = [ | ||
"disnake/", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this is also a shortcoming in CI. This should absolutely be discovered on the built wheels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added #1455 which won't necessarily catch this exact error but will catch other wheel errors.
if description is not None: | ||
fields.update(description.fields) | ||
fields["branch"] = description.branch | ||
if base_version is not None: | ||
fields["base_version"] = base_version | ||
if next_version is not None: | ||
fields["next_version"] = next_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template only uses {version}
and {version_template}
, are these actually needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its worth making sure that we're compatiable with the upstream implementation of this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to, these hooks are precisely to provide custom behavior separate from the upstream implementation. Nothing in the documentation says all these fields need to be retained, afaict.
try: | ||
fields["normalized_version"] = str(packaging.version.Version(version)) | ||
except ValueError: | ||
fields["normalized_version"] = version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, see previous comment
6594b5b
to
ff625d2
Compare
ff625d2
to
853d9ed
Compare
91ddca7
to
ddaa1f3
Compare
ddaa1f3
to
dbc301d
Compare
Without this setting, installation of disnake from a source distribution without source control metadata will fail, because there will be no way to determine the version of disnake without that metadata. Using a fallback ensures that in such an installation method (such as from a GitHub source archive), disnake can still be installed, though the version metadata may be inaccurate. Compared to not allowing installation at all, this is a better user experience, despite the incorrect version information.
1d0b983
to
a4ed52b
Compare
closes #664