On Sat, Feb 02, 2013 at 10:45:19AM -0800, Brian Dolbec wrote: > I do think that it will be difficult to make complete commits for > some changes that do not break catalyst until the remaining commits > are also merged. This may be true for some of the shifting commits, we'll see after the initial reroll. > Thank you for enough info for me to google about commit summary, > message body. Until now I didn't know about needing to separate > them with a blank line. I had never come across that info before, > so had given up trying to format them better. Everything I tried > had failed. Even googling it now it seems to be everywhere but > git's own help. Actually, it is in Git's help: $ git commit --help | grep -B3 blank DISCUSSION Though not required, it’s a good idea to begin the commit message with a single short (less than 50 character) line summarizing the change, followed by a blank line and then a more thorough description. The text up to the first blank line in a commit message is treated as the commit > > 321df3b whitespace cleanup > > > > This looks good, but there are also whitespace cleanups in some other > > commits (e.g. 2d36d29). I think we should just run something like: > > > > for FILE in $(git ls-tree -r --name-only HEAD | grep -v 'bz2$'); do > > sed -i 's/[[:space:]]*$//' "$FILE" > > done > > > > and have done with it ;). > > yeah, that requires way more knowledge about those tools than I know. > For me, I open a file, the editor does the cleanup automatically. If I > don't separate them when committing, then... If I didn't edit the > file... I've pushed this change to the `whitespace` branch on git://tremily.us/catalyst.git if you want to rebase your changes onto that. I can also submit it as a patch to the mailing list if anyone wants it that way. > > 38cd3bb add more configured defaults > > > > List the new settings (distdir, repo_name, packagedir, port_tmpdir, > > options, snapshot_name) in the commit message so I don't have to read > > the diff. Probably explain why you think they should be configurable > > as well. `options` is also a pretty ambiguous name. > > Now that I know how to blab on in a message body with a brief summary, > yeah :) > > "options" was existing and seems comparable to FEATURES for emerge in > make.conf. Suggestions for a new name? In a later comment I suggested breaking each of the options variables out into their own setting. Then we don't have to worry about what to call the boolean settings in aggregate ;). > > This might also be a good place to move the path construction over to > > use os.path.join(). > > Yes, I agree, but I am still getting accustomed to the code, so was > holding off that for a bit. Need to debug changes first. I am also > contemplating stealing the path() from layman which joins a list of > partial paths. I've noticed it could simplify code in many places that > join more than 2 partial paths. Doesn't os.path.join already do that? $ python2.7 -c "import os; print(os.path.join('a', 'b', 'c'))" a/b/c > > This might also be a good time to transition from `CatalystError, …` > > to `CatalystError(…)` where you're touching lines. > > > > I'm not sure which versions of Python Catalyst is trying to support, > > but I'd be happy to see the beginings of a migration to '{}'.format() > > for building strings (vs. the current `'' + ''` concatenation) > > yeah, there are a ton of those in the code. > > Since portage is python-2.6+ and python-2.5 is pretty close to being > removed from the tree... I intend to make it, 2.6, 2.7, 3.2 + > compatible. So the CatalystError(…) updates will be fine, and we should use '{0}'.format(xyz). > > 60919e4 Apply all Matt Turner's has_key() replacement patches > > > > This looks like a job for `rebase` ;). Cherry-picking will just lead > > to redundant commits. > > That will be looked after in my rebase to master again. Since I had > already made a number of changes in my rewrite branch, I could not apply > his commit cleanly without editing. Right. What you should have done is rebase onto the new `master` branch. Where Matt's changes clashed with yours, Git would stop and ask to to fix the conflicts. Then you could update *your* patches so that they worked ;). Since Matt's changes got into `master` first, he gets to keep them the way they originally were. > > 7a909b9 cleanup long lines, improve useage() output formatting > slightly > > > > Can we just switch from getopt to argparse (Python ≥2.7)? > > Yes, we can also add argparse as dep for 2.6 if they want to maintain > 2.6 compatibility. Ah, that would be much nicer :). > > 5eeb8b1 new minimal start script > > > > I think __version__ and __maintainer__ should live in > > catalyst/__init__.py. > > Can do. The version can also be set for releases easily using some > methods from gentoolkit when making the release tarball. The version > could then be left as "git" with a partial hash added of the last commit > when it was merged with. I do something like this in bugs-everywhere [1], but for Catalyst I think it may be more work than it's worth. > > 267dbb6 add __init__.py's to modules and arch sub-pkgs > > > > Added with blank lines? > > I cheated at the time and just used my editor to save an existing one to > different places. So it likely saved it with a blank line. Instead of > using a terminal and touch. These should probably get copyright blurbs and descriptive docstrings anyway, so it's probably not worth worrying over the blank lines. > > da6cbbf rename shell script targets directory to targets.sh so as to > not be confused with the new python targets directory > > > > “so as…” should go in the commit message body to get a shorter > > summary. > > > > I'm not sold on this. Since they are data files intended to be used > > by the Python modules, why not move them to catalyst/targets/scripts > [1]? > > I had been re-thinking that change and was thinking of renaming it back > to targets now that things have been better rearranged. There is one py > file in stage1. It is also easier to do now since it is just one edit > and a git mv away. I've removed that directory name from being hard > coded in the scripts, so now it just means there is one place to edit. This sounds reasonable to me. > > f9f18be update gitignore > > > > The Scratch, catalystc, *.geany, and test* entries would appear to be > > specific to your local installation and usage. I'd drop them from the > > reroll. > > > > They are only for my dev space git repo and me. They will not be part > of a final ready to merge branch. But my dev space is also acting as my > backup, so I committed them. It makes my git status output cleaner. Fair enough, but it's probably a good idea to put your local changes in a clearly marked commit: $ git commit -am 'FIXME: local changes to gitignore' > > d23d96b revert urllib import from commit > 64c16cae70da13de3c55d8555a2e4c5dcdf2fcad since it is not used anywhere > in catalyst > > > > 64c16cae does a lot more than import urllib, so I think “revert” gives > > the wrong impression. I'd remove the import entirely (instead of > > commenting it out), and say: > > > > catalyst/support.py: Remove unused urllib import > > > > This was introduced in commit > > 64c16cae70da13de3c55d8555a2e4c5dcdf2fcad but is no longer used. > > Actually urllib was never used. It was added as a fix for a bug. It > was not a correct fix, so just masked what was causing the bug. I only > commented it out until I could either find what caused the bug or it > just didn't return due to other changes. It is often easier to > comment/uncomment during testing runs than to remove/re-add code. Sure. It's strange that it had any effect at all on a bug if it was never used, but whatever ;). Cheers, Trevor [1]: http://gitorious.org/be/be/blobs/master/libbe/version.py#line21 http://gitorious.org/be/be/blobs/master/Makefile#line80 -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy