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

Add Description-Content-Type field #258

Merged
merged 16 commits into from
May 19, 2017

Conversation

@msabramo msabramo force-pushed the description-content-type branch 3 times, most recently from b9195d8 to c4cb911 Compare July 29, 2016 19:48
This allows specifying the content type of the distribution's
description (e.g.: `text/plain`, `text/x-rst`, `text/markdown`).
@nicktimko
Copy link
Contributor

Bikeshedding: CommonMark? Can that be the default?

@msabramo
Copy link
Contributor Author

msabramo commented Jul 30, 2016

Maybe. I've never used CommonMark; in fact, I hadn't heard of it until you mentioned it. Have a Python implementation that you like? https://pypi.python.org/pypi/CommonMark perhaps?

@msabramo
Copy link
Contributor Author

How is CommonMark different than the original Markdown? Why do you prefer it?

@nicktimko
Copy link
Contributor

CommonMark (http://commonmark.org/) is rigidly defined unlike Markdown which has bits of ambiguity (true "original" Markdown.pl is properly broken in some nesting cases). It improves a bunch of quirks, like spaces around lists, and standardizes some of the more convenient parts of GFM like fenced code blocks.

@msabramo
Copy link
Contributor Author

http://commonmark.org/ sounds interesting and has some big names behind it. I'm intrigued. I think I'll take a closer look and consider making CommonMark the default.

@msabramo
Copy link
Contributor Author

BTW, the only reason that I made original Markdown the default is because it's listed as the initial variant in RFC 7763 at https://tools.ietf.org/html/rfc7763#section-6.1.4, but then CommonMark is referenced in RFC 7764 at https://tools.ietf.org/html/rfc7764#section-4.5 (which I hadn't noticed before), so I guess nothing prevents it from being the default.

Part of me thinks that because there are so many competing Markdown variants and so much confusion around Markdown, that perhaps there should be no default and we should require the variant to be specified. Thoughts on that?

@ncoghlan
Copy link
Member

The overall default on the server side needs to be UTF-8 ReST (as in the current PR) for compatibility with existing projects that already correctly use ReST formatting.

However, when the media type is given as text/markdown, I'd actually suggest using either GFM (GitHub Flavored Markdown) or CommonMark (where GitHub are one of the backing organisations) as the default, as one of the more common reasons for using Markdown in README files these days is as a nice user-friendly front page for a GitHub repo.

So using CommonMark as the default (including falling back to it if asked to process an unknown variant) makes sense to me.

- ``text/markdown``

There is one required parameter called ``charset``, to specify whether the text
is UTF-8, ASCII, etc.
Copy link
Member

Choose a reason for hiding this comment

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

I'd make charset optional, defaulting to UTF-8 - that way text/markdown is sufficient to get Markdown rendering.

Copy link
Contributor Author

@msabramo msabramo Jul 30, 2016

Choose a reason for hiding this comment

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

My first instinct was to default to charset to UTF-8.

But then I saw this in RFC 7763 section 2:

     charset: Per Section 4.2.1 of [RFC6838], charset is REQUIRED.
     There is no default value because neither [MDSYNTAX] nor many
     popular implementations at the time of this registration do
     either.

That said, we can disagree with that RFC and do what we want, but I thought deferring to previous work would be a good start, since they probably had reasons and discussions around this.

Intuitively, it seems to me to be a good idea to simply default to UTF-8. I'm not sure why the RFC didn't simply do that. I wonder if they've thought of something that we haven't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I went ahead and made charset optional and defaulted it to UTF-8.

d454938

Copy link
Member

Choose a reason for hiding this comment

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

In the general case, I agree that there's no sensible universal default, but Python project descriptions aren't the general case - they're a specific use case. That lets us make assumptions in the name of ease-of-use that wouldn't be valid given a broader context.

@ncoghlan
Copy link
Member

+1 from me for the general idea, but what you have so far is missing guidance on expected behaviour for tools reading the field to render the description when:

  • they don't recognise the content type
  • they don't recognise the charset
  • they don't recognise the specific variant

Those additions would be SHOULD/MAY type guidelines rather than MUST requirements, but I think we still want to give some thought to how PyPI should handle those cases, and how those considerations can be turned into general recommendations.

msabramo added a commit to msabramo/setuptools_old that referenced this pull request Jul 30, 2016
This is used to populate the newly proposed `Description-Content-Type`
field, that is proposed in
pypa/packaging.python.org#258.
msabramo added a commit to msabramo/setuptools that referenced this pull request Jul 30, 2016
This is used to populate the newly proposed `Description-Content-Type`
field, that is proposed in
pypa/packaging.python.org#258.
@msabramo
Copy link
Contributor Author

Added mentions of CommonMark in 774d091.

@msabramo
Copy link
Contributor Author

Made CommonMark the default variant for Markdown in 33dc8c5.

@ncoghlan
Copy link
Member

Latest draft looks good to me, but I'll defer to @dstufft for actual sign-off.

While I don't expect this idea to be controversial, we should also give folks a chance to discuss the idea on distutils-sig before committing to it.

@msabramo
Copy link
Contributor Author

@ncoghlan: I'm still planning to add some info on expected behavior when things are omitted or invalid, as you suggested in #258 (comment).

But certainly, I'd love to have @dstufft and anyone else's comments.

One parameter is called ``charset``; it can be used to specify whether the
character set in use is UTF-8, ASCII, etc. If ``charset`` is not provided, then
it is recommended that the implementation (e.g.: PyPI) treat the content as
UTF-8.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move the sentence "If charset is not provided..." down below? (See next comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does e4b39db help?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @pfmoore that this can be dropped in favour of the more concise specification below.

@pfmoore
Copy link
Member

pfmoore commented Jul 31, 2016

Overall, LGTM (one minor comment in the diff).

for `Description-Content-Type` (and `charset` and `variant`)

as suggested by @ncoghlan at
pypa#258 (comment)
@msabramo
Copy link
Contributor Author

msabramo commented Jul 31, 2016

e4b39db: Clarified how to treat missing/invalid values for Description-Content-Type (and charset and variant) as suggested by @ncoghlan at #258 (comment)

@theacodes
Copy link
Member

theacodes commented May 17, 2017 via email

@dstufft
Copy link
Member

dstufft commented May 19, 2017

Sadly I'm not at PyCon, I just haven't gotten a chance to circle back around to this yet :(

@dstufft dstufft merged commit 42c2678 into pypa:master May 19, 2017
@msabramo
Copy link
Contributor Author

Yay, I see @dstufft merged this. Great news!

So what do we need to do next?

@dstufft
Copy link
Member

dstufft commented May 19, 2017

Probably implement support in setuptools, then wheel, then twine, then PyPI/Warehouse.

@nicktimko
Copy link
Contributor

Would it be better to have the server (PyPI/Warehouse) accepting it before the clients? Then newer clients wouldn't get errors if they support what the server doesn't

@ncoghlan
Copy link
Member

ncoghlan commented May 20, 2017

I think there are a few aspects with a required order, then some that can happen in arbitrary order:

  1. Check that pip will ignore the new field at install time. If not, update it, and document the minimum required version implied by using the new field. Also make note of whether or not easy_install will be able to install packages using the new field (we don't have to fix that if it does, but we should document the answer)
  2. Update PyPI/Warehouse to allow the new field (if they don't already)
  3. Update setuptools and the other build systems to allow setting the new field, and PyPI/Warehouse to actually pay attention to it

(We should probably document this somewhere on pypi.io as a standard practice, since it applies to any interoperability metadata changes we make)

ncoghlan pushed a commit to ncoghlan/packaging.python.org that referenced this pull request Jun 24, 2017
for `Description-Content-Type` (and `charset` and `variant`)

as suggested by @ncoghlan at
pypa#258 (comment)
ncoghlan pushed a commit to ncoghlan/packaging.python.org that referenced this pull request Jun 24, 2017
The former subject to a little agreement by others: pypa#258 (review)
ncoghlan pushed a commit to ncoghlan/packaging.python.org that referenced this pull request Jun 24, 2017
that was causing incorrect rendering

pointed out by @ekohl in pypa#258 (comment)
ncoghlan pushed a commit to ncoghlan/packaging.python.org that referenced this pull request Jun 24, 2017
di pushed a commit to di/setuptools that referenced this pull request Jul 3, 2017
This is used to populate the new `Description-Content-Type` field.

`Description-Content-Type` is described at
pypa/packaging.python.org#258.
di pushed a commit to di/setuptools that referenced this pull request Jul 3, 2017
Test that specifying a `long_description_content_type` keyword arg to
the `setup` function results in writing a `Description-Content-Type`
line to the `PKG-INFO` file in the `<distribution>.egg-info` directory.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
di pushed a commit to di/setuptools that referenced this pull request Jul 3, 2017
This is used to populate the new `Description-Content-Type` field.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
di pushed a commit to di/setuptools that referenced this pull request Jul 3, 2017
Test that specifying a `long_description_content_type` keyword arg to
the `setup` function results in writing a `Description-Content-Type`
line to the `PKG-INFO` file in the `<distribution>.egg-info` directory.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
di pushed a commit to di/setuptools that referenced this pull request Jul 3, 2017
This is used to populate the new `Description-Content-Type` field.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
di pushed a commit to di/setuptools that referenced this pull request Jul 3, 2017
Test that specifying a `long_description_content_type` keyword arg to
the `setup` function results in writing a `Description-Content-Type`
line to the `PKG-INFO` file in the `<distribution>.egg-info` directory.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
di pushed a commit to di/setuptools that referenced this pull request Jul 3, 2017
This is used to populate the new `Description-Content-Type` field.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
di pushed a commit to di/setuptools that referenced this pull request Jul 3, 2017
Test that specifying a `long_description_content_type` keyword arg to
the `setup` function results in writing a `Description-Content-Type`
line to the `PKG-INFO` file in the `<distribution>.egg-info` directory.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
di pushed a commit to di/setuptools that referenced this pull request Jul 24, 2017
This is used to populate the new `Description-Content-Type` field.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
di pushed a commit to di/setuptools that referenced this pull request Jul 24, 2017
Test that specifying a `long_description_content_type` keyword arg to
the `setup` function results in writing a `Description-Content-Type`
line to the `PKG-INFO` file in the `<distribution>.egg-info` directory.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
di pushed a commit to di/setuptools that referenced this pull request Aug 28, 2017
This is used to populate the new `Description-Content-Type` field.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
di pushed a commit to di/setuptools that referenced this pull request Aug 28, 2017
Test that specifying a `long_description_content_type` keyword arg to
the `setup` function results in writing a `Description-Content-Type`
line to the `PKG-INFO` file in the `<distribution>.egg-info` directory.

`Description-Content-Type` is described at
pypa/packaging.python.org#258
@ncoghlan ncoghlan mentioned this pull request Dec 4, 2017
@wimglenn
Copy link
Contributor

wimglenn commented Dec 5, 2017

@ncoghlan or anyone else who knows about this, could you please explain how to get it working properly? I added in setup.cfg:

[metadata]
long_description_content_type = text/x-rst; charset=UTF-8

This is syntax documented as supported in setuptools (here). But it doesn't seem to be respected or used for anything - the generated metadata in the distribution always just shows "UNKNOWN". Is the correct value getting dropped in the installation at some stage?

I use the version of setuptools v38.2.4, and I use "python setup.py bdist_wheel" to build the dist.

@di
Copy link
Member

di commented Dec 5, 2017

Hi @wimglenn, there's an open PR to setuptools to fix this: pypa/setuptools#1207

@wimglenn
Copy link
Contributor

wimglenn commented Dec 6, 2017

@di ah, yes, that’s exactly the spot I ended up at in pdb when I was looking at it. Thanks!

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

Successfully merging this pull request may close these issues.