From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id B6B57138800 for ; Sat, 2 Feb 2013 18:47:01 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 3FA1BE001E; Sat, 2 Feb 2013 18:47:00 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 9DC17E001E for ; Sat, 2 Feb 2013 18:46:59 +0000 (UTC) Received: from [192.168.1.210] (unknown [24.86.176.233]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: dolsen) by smtp.gentoo.org (Postfix) with ESMTPSA id AAE3533DD20 for ; Sat, 2 Feb 2013 18:46:58 +0000 (UTC) Message-ID: <1359830719.3997.63.camel@big_daddy.dol-sen.ca> Subject: Re: [gentoo-catalyst] More proposed Catalyst changes From: Brian Dolbec To: gentoo-catalyst@lists.gentoo.org Date: Sat, 02 Feb 2013 10:45:19 -0800 In-Reply-To: <20130131183922.GA3946@odin.tremily.us> References: <1357633976.4289.61.camel@big_daddy.dol-sen.ca> <1357980958.4289.82.camel@big_daddy.dol-sen.ca> <20130131183922.GA3946@odin.tremily.us> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.6.3 Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-catalyst@lists.gentoo.org Reply-to: gentoo-catalyst@lists.gentoo.org Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Archives-Salt: 9d7e9415-ac6b-4015-b014-dff4ad92d132 X-Archives-Hash: 99084eba8a3f3352fa095fd47d52e2d7 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 >