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