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

CHANGES.rst is very uninformative #1015

Closed
homm opened this issue Nov 19, 2014 · 9 comments
Closed

CHANGES.rst is very uninformative #1015

homm opened this issue Nov 19, 2014 · 9 comments

Comments

@homm
Copy link
Member

homm commented Nov 19, 2014

There are only two words about each change. Also it includes changes not related to library usage. For example:

Fix Bicubic interpolation

Actually, bicubic was not broken, because im.stretch was newer used with bicubic before.

Fix antialias compilation on debug versions of Python

This bug was introduced in master and was newer in any of release or beta versions.

Ico save, additional tests

Additional tests not change in library itself, not that would be useful or interesting to users.

I'm not speaking about ideal Django changes for each release. But look, at original PIL changes, it full of details. One example:

Added NumPy array interface support (array_interface) to the Image class (based on code by Travis Oliphant).
This allows you to easily convert between PIL image memories and NumPy arrays:

import numpy, Image
im = Image.open('lena.jpg')
a = numpy.asarray(im) # a is readonly
im = Image.fromarray(a)
@hugovk hugovk changed the title Changes.rst is very uninformative CHANGES.rst is very uninformative Nov 19, 2014
@hugovk
Copy link
Member

hugovk commented Nov 19, 2014

I generally put the name of the pull request that was merged, sometimes edit it a bit. I've also started adding the PR number so you can always look up the full explanation, diff, and discussion (including links to original issue reports and Travis builds etc.), which I think coupled with a descriptive one-liner is a good way of avoiding the README getting too big.


Fix Bicubic interpolation

Actually, bicubic was not broken, because im.stretch was newer used with bicubic before.

From CHANGES.rst:

PR #970 is titled "Fix bicubic stretch interpolation" so it most likely came from there. I suggest naming PRs how you'd like them to appear in CHANGES. Something concise and descriptive.


This bug was introduced in master and was never in any of release or beta versions.

I suppose we don't really need to mention these either, but doesn't bother me too much.


Ico save, additional tests

Additional tests not change in library itself, not that would be useful or interesting to users.

#1007 included library and tests changes. I don't have a problem mentioning tests (even test files) as well, because a well-tested library is better than an untested one, and considerable work goes into writing and maintaining tests as well as code. Is it ever useful for us to read CHANGES and see things about tests, and perhaps even build/project infrastructure if necessary?


But this is a good time to consider: Who is the intended audience of CHANGES.rst? What do they need to see? Are we part of this audience? What should CHANGES.rst contain?

We don't include the whole CHANGES on PyPI because it got too long and caused problems for packagers, but we do link to it from there.

@homm
Copy link
Member Author

homm commented Nov 19, 2014

PR #970 is titled "Fix bicubic stretch interpolation" so it most likely came from there.

But there is no "stretch" word in changelog. And even if it was there, it is very hard to understand what exactly was fixed without a brief explanation, which will not fit in PR title.

Who is the intended audience of CHANGES.rst? What do they need to see?

I believe changes file for any library should answer to following questions:

  • What new functionality were added, how can I use it in my project
  • What bugs were fixed, so which workarounds and restrictions I can remove from my project
  • What behavior were changed, so I can check is behavior correct for me

Django release notes is great example of changelog. Of course, we don't need so detailed changelog. But if you are suggesting to open PR for each line (which is even not a link, by the way!), how is it better then simple list of PR numbers?

@wiredfool
Copy link
Member

generally put the name of the pull request that was merged, sometimes edit it a bit. I've also started adding the PR number so you can always look up the full explanation, diff, and discussion

This is pretty much what I'm doing.

I'm not sure changes.rst is really working for anyone, other than that it's a somewhat digested list of what made it into each release.

The individual commit messages are way too verbose, not in a logical order, and contain bits that tend to be backed out and redone. They're going to stay that way unless we aggressively rebase/squash each PR into a single non-merge commit. I'm not sure that's a good way to work with git, but it would clarify the history and chunk the changes into logical units.

We're missing a list of 'Here's what's important that changed'. I like how PostgreSql does it, in that there's a list of changes that you need to know about when upgrading from previous versions, and then a full change log that contains a lot of the other detail about things added and bugs fixed. They're not chronological, rather organized by section of the software.

@homm
Copy link
Member Author

homm commented Nov 19, 2014

Maybe we should add release notes then?

@hugovk
Copy link
Member

hugovk commented Nov 20, 2014

The individual commit messages are way too verbose, not in a logical order, and contain bits that tend to be backed out and redone. They're going to stay that way unless we aggressively rebase/squash each PR into a single non-merge commit. I'm not sure that's a good way to work with git, but it would clarify the history and chunk the changes into logical units.

Yeah, I don't think it's worth the effort. It may be useful to have those individual commits for the handy git bisect.

@aclark4life
Copy link
Member

This is not a problem unique to Pillow (i.e. describing complex development changes in human readable form) and typically requires someone to do time consuming and tedious work. As such, I'm satisfied with a snippet that mentions important keywords (e.g. 'Fixed bug' or 'Refactored function' or 'Added support for' and references an issue (not a commit). Without investing a lot of time, I think that's the best we can do here. And folks should be able to skim CHANGES.rst to see if something they care about changed, then do the research to find out more. To say that a CHANGES.txt entry was "not accurate" somewhat misses the point: it doesn't have to be. Something changed and readers can figure the rest out themselves. Would it be nice to do better? Of course.

For whatever it's worth, the only thing I'm currently not happy about was that CHANGES.rst was completely removed from the long_description in setup.py. I personally don't enjoy having to click on a link to see changes, but I'm willing to live with that for now. If I were to improve, I'd suggest making CHANGES.txt include release notes for the current release and archive the remainder elsewhere.

@homm If you want to alter the status quo, please send a PR! (And that PR could be sent to the release documentation we have somewhere, e.g. to suggest an alternate practice.)

@wiredfool
Copy link
Member

I was thinking about this (before this issue) that we really need a distilled set of "This is what's important in this release" from the point of view of what's going to break existing scripts. At this point for this release, I'd probably just have that gaussian blur radius is interpreted differently now.

I could see putting a Release Notes with that info, and splitting out current changes from archived changes, so that they could be readded to the description in pypi. I don't think that it would be that much work.

@aclark4life
Copy link
Member

@wiredfool Great, carry on then 😄 And as always, please let me know if you need anything from me to help with this or any task.

@homm
Copy link
Member Author

homm commented Dec 7, 2014

Fixed in #1032 and subsequent.

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

No branches or pull requests

4 participants