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

Add setup.cfg support for long_description_content_type #1207

Merged
merged 2 commits into from Jan 5, 2018

Conversation

Projects
None yet
5 participants
@cryvate
Contributor

cryvate commented Nov 21, 2017

This was already claimed in the docs. That this did not work before can be seen by cherry picking the changed test in this PR without the other changes: it will fail. Fixes issue #1206

@fungi

This comment has been minimized.

Contributor

fungi commented Nov 22, 2017

I was just noticing this today while working on Project-URL metadata support, and your try/except there is exactly what I was missing to get my added test passing. Thanks!

@@ -332,6 +332,13 @@ def __init__(self, attrs=None):
for ep in pkg_resources.iter_entry_points('distutils.setup_keywords'):
vars(self).setdefault(ep.name, None)
_Distribution.__init__(self, attrs)
try:

This comment has been minimized.

@jaraco

jaraco Nov 25, 2017

Member

This code block bothers me for several reasons.

First, it seems overly defensive. Is it really true that sometimes metadata.long_description_content_type will be set, but other times not? Even if that's really true, it should use vars(self.metadata).setdefault('long_description_content_type', self.long_description_content_type.

Second, why is long_description_content_type being singled-out here? Why do other metadata fields not need this special handling?

Third, it seems bad form to be copying an attribute by value from one object to another. Ideally, and especially for newer attributes, there should be one place where that value is defined.

I think perhaps the bug that this PR is seeking to address is revealing a design flaw in the implementation of the attribute.

Can we make long_description_content_type work like other attributes so it doesn't require this special treatment?

This comment has been minimized.

@cryvate

cryvate Nov 25, 2017

Contributor

This code block does not make me happy. I know that distutils' DistributionMetadata will not set this attribute, and setup.cfg is not honoured because of this check. I think it is different because it does not appear in any PEP, and hence not in distutils.

I'll have a look at making things more sleek :) I think there will still need to be special casing, but I will at least try to reduce it down to one place.

This comment has been minimized.

@cryvate

cryvate Nov 25, 2017

Contributor

I think fundamentally, the only way not to special case it, is to patch distutils' DistributionMetadata. As you can see in the new commit I just pushed, there was some special casing of this attribute that is slightly simplified (in my view) by setting it in the constructor.

@@ -598,7 +598,7 @@ def write_pkg_info(cmd, basename, filename):
metadata.version, oldver = cmd.egg_version, metadata.version
metadata.name, oldname = cmd.egg_name, metadata.name
metadata.long_description_content_type = getattr(
cmd.distribution,
cmd.distribution.metadata,

This comment has been minimized.

@jaraco

jaraco Nov 25, 2017

Member

This probably should just be metadata.

@jaraco jaraco merged commit 23b940e into pypa:master Jan 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jaraco added a commit that referenced this pull request Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment