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

Allow combining several files in metadata.long_description #1131

Merged
merged 12 commits into from Aug 28, 2017

Conversation

@webknjaz
Copy link
Member

@webknjaz webknjaz commented Aug 9, 2017

This basically allows one to include several files into long_description via setup.cfg in a declarative way.

Use case

Sometimes one wants to have their README and CHANGELOG stored as separate files, so that GitHub would only render README contents on a main repo page. But it is nice to list changes on PYPI as well.

For such cases I suggest parsing long_description config value as a list with possibility to inject static files contents via file: directive.

This would've been possible with RST .. include::, but setuptools doesn't process RST internally. So it's not an option.

In general, suggested change doesn't break compatibility.

/cc: @jaraco it would be interesting to see your feedback :)

@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented Aug 22, 2017

@jaraco ping

Loading

jaraco
jaraco approved these changes Aug 26, 2017
Copy link
Member

@jaraco jaraco left a comment

Overall this looks good. I suspect this approach will work as well with markdown as rst, so shouldn't run into any conflicts in that regard. I would like to get an additional review from someone more familiar with the declarative config.

Loading

@@ -408,7 +408,7 @@ def parsers(self):
'classifiers': self._get_parser_compound(parse_file, parse_list),
'license': parse_file,
'description': parse_file,
'long_description': parse_file,
'long_description': self._get_parser_compound(parse_list, lambda l: '\n'.join(map(parse_file, l))),
Copy link
Member

@jaraco jaraco Aug 26, 2017

Choose a reason for hiding this comment

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

In place of the lambda, I would use a new method, self.parse_multi_file or similar.

Loading

Copy link
Member Author

@webknjaz webknjaz Aug 27, 2017

Choose a reason for hiding this comment

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

sounds fair

Loading

@@ -245,8 +245,8 @@ def _parse_file(cls, value):
directory with setup.py.
Examples:
include: LICENSE
include: src/file.txt
file: LICENSE
Copy link
Member

@jaraco jaraco Aug 26, 2017

Choose a reason for hiding this comment

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

I presume this is a correction and not a change.

Loading

Copy link
Member Author

@webknjaz webknjaz Aug 27, 2017

Choose a reason for hiding this comment

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

yes

Loading

@jaraco
Copy link
Member

@jaraco jaraco commented Aug 26, 2017

@idlesign Would you review this PR?

I would add you as a reviewer, but I've only just invited you to the pypa org, so I can't add you as a reviewer formally.

Loading

@idlesign
Copy link
Member

@idlesign idlesign commented Aug 27, 2017

@jaraco Sure. Thank you.

@webknjaz, @jaraco I'd like to discuss it. I don't think file directive interpolation is a good approach, because it won't be easy to port to PEP 518 (if/when time comes); it'll provoke users putting formatted texts in setup.cfg; the problem seems be be solvable in a more generic way.

When I've started this declarative thing, I've been thinking about file: (which was include: at the beginning, thank you for docstring fix by the way) accepting multiple comma-separated arguments:

long_description = file: README.rst, CHANGELOG

This would allow file contents concatenation in a simple manner. Note that we deliberately simplify directive by not accepting joiner string, which is \n in this very pull request; that's because formatting and headings are usually done in files themselves.

The reason it has not been implemented is my personal dislike of very long descriptions: I believe in that what we see in cheese shop should be concise and changelogs (and license texts for that matter) don't really belong there — see how long-paced https://github.com/pypa/warehouse puts it on a separate Release History page.

Now I'd like to hear you thoughts.

Loading

@jaraco
Copy link
Member

@jaraco jaraco commented Aug 27, 2017

I also used to include the changelog as a matter of course in my packages but found that it grew unwieldy after a few releases, so eventually dropped changelogs from the package metadata and left the either in the repository or published with the docs. Still, I think it's common enough that it might be nice to support.

I also like the constraint that @idlesign's proposal suggests, which allows for multiple files, but doesn't introduce a mixed-mode design (where some content is in files and some content is inline).

Loading

@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented Aug 28, 2017

Well, I'd add that sometimes people may want to add a delimiter between files and include those files into Sphinx docs, for example, in a different way. Does it make sense to create additional one-liner file in such case?

P.S. @jaraco @idlesign I'll push suggested changes soon. Thanks for your feedback!

Loading

@idlesign
Copy link
Member

@idlesign idlesign commented Aug 28, 2017

I personally see no need in separate files in next to all cases (see my note above) — headings from files are quite delimiters. Is there a real life use-case?

Loading

@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented Aug 28, 2017

long_description = file: README.rst, CHANGELOG

What if there's a comma in filename?

Loading

@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented Aug 28, 2017

Is there a real life use-case?

I saw code stripping .. :changelog: off changelog, like this:

with open('CHANGELOG.rst') as CHANGELOG_file:
    CHANGELOG = CHANGELOG_file.read().replace('.. :changelog:', '')

So I assume there would be a need for some custom heading within PYPI description, which would differ from what's in sphinx docs.

Loading

@idlesign
Copy link
Member

@idlesign idlesign commented Aug 28, 2017

What if there's a comma in filename?

FileNotFoundError, as we do not encourage such office-exotic %)

.. :changelog:

Sorry, I'm not acquainted with this directive and can't see how it legitimately can replace good-old headers. Can you share a link to such docs?

Loading

@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented Aug 28, 2017

Me neither, I've just copy-pasted this from a project I happen to review (it looks like it comes from here, probably sphinx understands it somehow), but my point here is that threre may be some cases where people may actually want to do such things.

Loading

* `file:` not accepts comma-separated list of filenames
* files' contents are glues with an LF separator
@webknjaz webknjaz force-pushed the feature/long-desc-few-files branch from 6f34441 to b699f94 Aug 28, 2017
@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented Aug 28, 2017

@jaraco @idlesign I've updated the PR as you requested. Feel free to review again.

Loading

@idlesign
Copy link
Member

@idlesign idlesign commented Aug 28, 2017

[...] probably sphinx understands it somehow [..]

My guess that this is a bogus link target, yet I see no point in replacing it since Sphinx will happily ignore unknown directives. Note that changelog heading is still in place.

[...] but my point here is that threre may be some cases where people may actually want to do such things.

I understand that, but there's no reason neither way to make all people happy.

Loading

@idlesign
Copy link
Member

@idlesign idlesign commented Aug 28, 2017

I've updated the PR as you requested. Feel free to review again.

Thank you. I'm OK with it. See Travis hints, docs update would be nice too.

Loading

webknjaz added 2 commits Aug 28, 2017
Now it's possible to use it like this:
* long_description = file: README.rst, CHANGELOG.rst, LICENSE.rst
@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented Aug 28, 2017

I've updated docs.

Loading

jaraco added 5 commits Aug 28, 2017
If one wishes to test both 'mixed' and 'sandboxed', let's create a separate test for that, but it's probably unnecessary.
self._read_local_file(path)
for path in filepath
if os.path.isfile(path)
)
Copy link
Member Author

@webknjaz webknjaz Aug 28, 2017

Choose a reason for hiding this comment

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

@jaraco

You changed the behavior here. If no file was read, the method returned original value, not an empty string.

This would correct the flow:

return (...) or value

Loading

Copy link
Member

@jaraco jaraco Aug 28, 2017

Choose a reason for hiding this comment

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

Hmm. I'll have to look more closely.

Loading

Copy link
Member

@jaraco jaraco Aug 28, 2017

Choose a reason for hiding this comment

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

Although, if the tests aren't capturing that use-case, then it's not supported.

Loading

"""
if not filepath.startswith(os.getcwd()):
raise DistutilsOptionError(
'`file:` directive can not access %s' % filepath)
Copy link
Member Author

@webknjaz webknjaz Aug 28, 2017

Choose a reason for hiding this comment

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

I would prefer that file: directive name part would be rendered from some reusable constant in this message.

Loading

Copy link
Member

@jaraco jaraco Aug 28, 2017

Choose a reason for hiding this comment

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

Yes, perhaps. This change makes that more difficult.

Loading

Copy link
Member

@idlesign idlesign Aug 28, 2017

Choose a reason for hiding this comment

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

[...] from some reusable constant [...]

Mind Occam's razor.

Loading

Read contents of filepath. Raise error if the file
isn't in the current directory.
"""
if not filepath.startswith(os.getcwd()):
Copy link
Member Author

@webknjaz webknjaz Aug 28, 2017

Choose a reason for hiding this comment

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

os.getcwd() generates a syscall on each loop iteration. It would be preferable to call it once in __init__() and then reuse saved value.

Loading

Copy link
Member

@jaraco jaraco Aug 28, 2017

Choose a reason for hiding this comment

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

Sounds like a premature optimization to me. The added complication of the logic doesn't justify the rare, few syscalls.

Loading

Copy link
Member Author

@webknjaz webknjaz Aug 28, 2017

Choose a reason for hiding this comment

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

I saw dudes on the Internet complaining that setuptools is slow, that's why this came to my mind.

Loading

@jaraco jaraco merged commit fffd844 into pypa:master Aug 28, 2017
2 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants