Message106851
I’ve started to review your patch but I find it a bid unwieldy to read. Would you mind exporting changesets to patches? I tried to look at your repo, but didn’t know which branch to look at.
So, thanks for giving us the occasion of thinking about the patch workflow. Using changesets seems more natural since we use Bitbucket, while OTOH using regular diffs can streamline the history and remove unnecessary details (e.g. I’m not sure we want branch names to clutter the history forever (see http://stevelosh.com/blog/2009/08/a-guide-to-branching-in-mercurial/ for more info)). Would it be too much of a hassle to provide links or a Mercurial bundle of a set of small changesets? Having different patches would help us separate the non-controversial stuff (whitespace) from the refactoring and improvements.
That said, I can offer a few remarks on the patch.
- Don’t put editor settings in the files.
- Changing “if s in 'yn'” to “if s in ('y', 'n')” is not really an improvement. It’s not more readable to always use tuples or frozensets for membership testing; str has __contains__ for a reason :)
- Not sure how your print_ indirection is helpful.
- I don’t know how much reST we want in the docstrings: None, some constructs, Sphinx extensions?
Don’t worry if there are no replies in a few days, people are pretty busy. I’ll ask about the meta-issues (workflow, reST) on the ML if Tarek doesn’t answer here. Cheers :) |
|
Date |
User |
Action |
Args |
2010-06-01 17:56:52 | eric.araujo | set | recipients:
+ eric.araujo, jafo, tarek, meatballhat |
2010-06-01 17:56:52 | eric.araujo | set | messageid: <1275415012.12.0.149529474732.issue8591@psf.upfronthosting.co.za> |
2010-06-01 17:56:49 | eric.araujo | link | issue8591 messages |
2010-06-01 17:56:48 | eric.araujo | create | |
|