On Sat, Feb 02, 2013 at 12:41:32PM -0800, Brian Dolbec wrote: > On Thu, 2013-01-31 at 14:46 -0500, W. Trevor King wrote: > > 968d818 Initial creation of a defaults file. Split out hash and > > contents to their own classes, files > > > > After this, it's not clear to me what the difference is between > > catalyst.support and catalyst.util. Perhaps they should be merged. > > I hadn't looked at util until now. > > hmm, I wonder if it would be better to move CatalystError there and > rename it to error.py There is only a couple error traceback functions > in it. I was thinking it might be good to define a few more specific > error classes than just the general CatalystError(). But I am not yet > familiar enough with the code to know for certain.
This sounds good to me too.
> > a4ef493 re-version to "git-rewrite branch"
> >
> > Why?
>
> Why not. It is not intended to be pushed into master. It also helps to
> confirm that you are running the correct code. I have fixed it so the
> code can be run from the git checkout. That way you can have a
> "Production" version installed on the same system. Now that I am
> testing/debugging, I am comparing catalyst operation and results to
> current -9999 code. I am not familiar with using catalyst, so that is
> helping me figure out what is wrong.
A fair enough. Again, it would be nice if there was a FIXME or
something in the commit message so I knew it wasn't destined for
master ;).
> > 9d752a7 move confdefaults out of main.py
> >
> > Looks good, except, I'm not sure why you changed from
> > `confdefaults.keys()` to `list(confdefaults)` in parse_config() (which
> > should probably be living in catalyst.config anyway).
>
> keeping a separate defaults file can be helpful in importing some info
> into different modules while keeping imports to a minimum. Sometimes it
> helps prevents circular import problems. At this point I opted for a
> separate file, to be determined later if a merge is warranted.
ok.
> As for list(confdefaults), py3 compatibility. dict.keys() isn't usable
> and 2to3 converts it to list(dict)... something about needing to specify
> the return type. So is a preemptive change. One less thing to change
> later.
Really?
$ python3.3 -c "a = {1:2, 3:4}; print([x for x in a.keys()])"
[1, 3]
On the other hand, it might be cleaner to just say:
for x in confdefaults:
But this should still go into a separate commit.
> > c303dae some options cleanup, unifying their use, reducing redundancy.
> >
> > While I like the general thrust of this, I'd be happier with explicit
> > boolean options instead of a set of boolean options. For example:
> >
> > confdefaults = {
> > 'autoresume': False,
> > 'ccache': False,
> > …
> > }
>
> Yeah, I removed those. They were capitalized versions of the values in
> options. So, I optimized them into options becoming a set which
> eliminates, the duplication and potential problems by changing the value
> of one and not the other. Believe me keeping 2 different lists in sync
> can be much more difficult than it seems. It also makes things much
> more difficult to debug. (the independent booleans like you suggest can
> be considered a list, the other is the options list, set,
> string...whatever form it is in)
I think I would do something like:
import collections as _collections
import ConfigParser as _configparser
CONFIG = _configparser.ConfigParser(dict_type=_collections.OrderedDict)
for setting,value in [
('ccache', str(False))]:
CONFIG.set('DEFAULT', setting, value)
although OrderedDict doesn't exist in 2.6, where we should probably
just fall back to dicts. Use it with:
if CONFIG.getboolean('DEFAULT', 'ccache'):
…
Then there's no duplication to worry about, and you get a
configuration object Python developers will be familiar with.
> also using member inclusion is faster and prefered compared to other
> methods like has_key(). In this case, it is just simpler to use
> "options" rather than to individualize them.
I don't think option lookup speed will have much impact on catalyst
execution speed ;). And with ConfigParser, individual options will
require no additional coding.
> > Can we use logging instead of print?
>
> YES!!!!, please :D
>
> that's been on my wish list too.
I can add this if you don't want to. Let me know if you want me to
base my patch against `master`, or against something in your branch.
> > I think keyword arguments are better, because changes to keywords
> > usually occur alongside changes to the argument semantics. A keyword
> > mismatch is an obvious fix, while changes due to a semantic shift can
> > be more subtle.
>
> yeah, I was a bit frustrated at that point, debugging code, so chose the
> easy way. /me fixes.
;). I've certainly been there too.
> > 923e8a2 remove trailing slash for consistency in variables and remove
> > extra slashes in paths
> >
> > os.path.join()
>
> Yes, for sure. But after I debug my current changes. But also most of
> those are for the bash side consistency which can not use os.path.join()
> and were adding the slashes again at times.
>
> I am not a bash programmer, so if someone good at the bash stuff wants
> to work on those... go for it. I'll try to keep my damage to a minimum.
I can look into this, but I think I need a better description of the
problem first. Can you give me an example breakage due to using
os.path.join?
> Also if releng want to recode them in python, that's ok with me ;)
I'd be ok with that too, although I don't feel a pressing need for it
;).
> Thank you for the review, it has been informative. And good to keep me
> from any blunders.
No problem :D
Trevor
--
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
