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
signature.asc
Description: OpenPGP digital signature
