Re: [gentoo-catalyst] More proposed Catalyst changes

[Brian Dolbec]

On Thu, 2013-01-31 at 13:39 -0500, W. Trevor King wrote:
> On Sat, Jan 12, 2013 at 12:55:58AM -0800, Brian Dolbec wrote:
> > You can get a git clone of my rewrite work at:
> > http://dev.gentoo.org/~dolsen/catalyst/
> >
> > rewrite branch
>
> Ah, it's good to see things looking more Pythonic in Catalyst-land ;).
> Here's my initial review to push this forward, although I'm not
> particularly well-versed in the Catalyst internals.  This is branch is
> getting to the stage where rebasing will get annoying, so I think
> merging stuff that looks good into the master branch should happen
> sooner, rather than later ;).
>

There are a few fixes that should be for sure merged into master.  The
rest I intend to rebase them into more meaningful complete commits. 

A couple general answers for questions in this and your next review
email:


 I am halting further changes at this point except for what's needed to
debug everything up to this point.  I will fix/rebase commits to clean
them up as much as possible.  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.


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.




> 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...

>
> 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?

>
> 016704a use the new configured snapshot_name and portdir settings
>
> Rather than a separate add (38cd3bb) and use (016704a), I think it
> would be better if new options had their addition and use rolled into
> a single commit (e.g. add snapshot_name and use it as one commit, add
> portdir and use it as another commit).

I will in the rebase, making new commits.

>
> 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.

>
> fefd501 use the portdir setting rather than hard-coded path
>
> Good to merge.
>
> 63a25eb Remove self.mounts and self.mountmap's use of paths for keys
and paths. Migrate more hardcoded paths to use settings.
>
> That's a long commit summary ;).  Perhaps the “Migrate…” portion
> should go into the commit message body, or that could be split out
> into a separate commit.

will fix

>
> 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. 

>
> 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.



> 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.

>
> af6e6b7 Initial rearrangement of the python directories
>
> Hooray!
>
> 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.


> 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.

>
> 299e35d update the module loading paths for the new locations
>
> Can we just drop this import manipulation and use __all__?
>
> e26de08 chmod +x
>
> Squash into 5eeb8b1.
>
yup

> a7206bb rename files directory to etc to better reflect the
directories contents
>
> Actually, files/ is also used for other things (e.g. built man pages,
> see MAN_PAGES in the Makefile).  If we keep the rename to etc/, we
> might to just build the man pages under doc/.
>
Sounds like a plan to me.

> 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.


> 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.


> 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.


> ab2c22a comment out some code introduced in commit
b3475906d5f51a21ecaf4ff048002a2f44face52 with an undefined variable "s".
The code must always be hitting the general except: pass block.  Seems
useless, if not maybe it'll spit out a true error, so the real problem
can be fixed
>
> Multiple sentences in a commit summary!  Also, take advantage of your
> version control and just rip the code out instead of commenting it
> out.

I did that because I did not fully figure out what that whole code block
was suppose to do and refactor it all.  Leaving it commented out leaves
it there for that purpose later which also stands out as a reminder.


 I looked through b347590 (which is a 408-line monster for
> catalyst_support.py), but I'm not clear why Eric was messing with that
> code.  At any rate, it looks like a good cantidate for replacement
> with subprocess.Popen, although see 1b2597e ;).

Yeah, there are a lot of os.system() calls throughout the code to
replace.

>
> 95704c7 fix an undefined variable
>
> Oops ;).  I'd mention DESIRED_RLIMIT in the commit message.
>

yeah.

> 1b2597e update todo for a spawn() cleanup.
>
> +lots
>

yeah, there are lots of things to add to the todo.  I just didn't want
to forget about that one.  But if others are going to help, keeping that
up to date will be more important.


> Cheers,
> Trevor
>
> [1]:
http://docs.python.org/2/distutils/setupscript.html#installing-package-data
>