Re: [gentoo-catalyst] More proposed Catalyst changes

[W. Trevor King]

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