STY: Wrap __all__ to one entry on each line, and add trailing commas #9453
Conversation
Having this line-wrapped by column causes needless merge conflicts.
Done by repeated application of s/(__all__ = \[(?:\n '\w+',)+) ('\w+')/\1\n \2/g
|
I'd sort them and collapse them again. I think merge problems due to |
This isn't helpful, because then people just add new functions in a random place, because they don't want to rewrap all the lines. The only use in sorting it would be if we intend to always keep it sorted.
This is fine for recent PRs, but for old ones where the author no longer cares, this requires one of us to checkout and rebase the branch, which is a lot harder than just hitting the merge button. I suppose this is already a problem with targeting the wrong release notes, but that too can be fixed by using the matplotlib approach of creating separate rst files per feature, and merging them upon release |
|
Being less diplomatic, the single entry per line is unusual and, IMHO, really ugly ;) Release notes are a problem worth looking into, although I'm not convinced that the matplotlib example is the way to go. It would be nice to automate it more, maybe by adding a release notes section to the PR. Not sure that we can do that though. Maybe there are some hooks. Or it could be added to the PR description with a template and then parsed when the merged PR's are put together for the changelog. That wouldn't cover all sections but might take care of the busiest ones. |
| 'ascontiguousarray', 'asfortranarray', 'isfortran', 'empty_like', | ||
| 'zeros_like', 'ones_like', 'correlate', 'convolve', 'inner', 'dot', | ||
| 'outer', 'vdot', 'roll', | ||
| 'rollaxis', 'moveaxis', 'cross', 'tensordot', 'array2string', |
There was a problem hiding this comment.
I would argue that randomly wrapped things like this are more ugly
There was a problem hiding this comment.
Easy enough to fix up now and then. But I think this PR solves a mostly non-problem.
|
I think I have to agree with Chuck. Don't see much of a problem here and the massive lines of code are also annoying/not pretty in my eyes. I am happy about resorting them alphabetically, but it can also stay as fly by maintenance as now. I am pretty sure it would be relatively easy to add a test/style check to Travis to check for |
|
I am indifferent to how The basic idea is that each PR includes a little file containing its news entry dropped into a special directory, so they can be reviewed along with the patch, carried along with backports, etc., and then at release time you run a command to automatically bundle these up into the documentation. |
|
@eric-wieser I'm going to close this unless you want to argue about it some more ;) |
|
I wouldn't say it's a non-problem, but it is a minor one - and it sounds like other contributors consider the line noise a larger one. Unless anyone else feels the same way as me, closing this sounds like the right approach |
|
OK, closing. |
Having this line-wrapped by column causes needless merge conflicts.
Done by repeated application of
s/(__all__ = \[(?:\n '\w+',)+) ('\w+')/\1\n \2/gPros:
git blamebecomes useful for seeing when things were added tofrom numpy import *Cons:
Would be reasonably straightforward to sort these too, if desirable