1 |
On Sat, Feb 02, 2013 at 12:41:32PM -0800, Brian Dolbec wrote: |
2 |
> On Thu, 2013-01-31 at 14:46 -0500, W. Trevor King wrote: |
3 |
> > 968d818 Initial creation of a defaults file. Split out hash and |
4 |
> > contents to their own classes, files |
5 |
> > |
6 |
> > After this, it's not clear to me what the difference is between |
7 |
> > catalyst.support and catalyst.util. Perhaps they should be merged. |
8 |
> |
9 |
> I hadn't looked at util until now. |
10 |
> |
11 |
> hmm, I wonder if it would be better to move CatalystError there and |
12 |
> rename it to error.py There is only a couple error traceback functions |
13 |
> in it. I was thinking it might be good to define a few more specific |
14 |
> error classes than just the general CatalystError(). But I am not yet |
15 |
> familiar enough with the code to know for certain. |
16 |
|
17 |
This sounds good to me too. |
18 |
|
19 |
> > a4ef493 re-version to "git-rewrite branch" |
20 |
> > |
21 |
> > Why? |
22 |
> |
23 |
> Why not. It is not intended to be pushed into master. It also helps to |
24 |
> confirm that you are running the correct code. I have fixed it so the |
25 |
> code can be run from the git checkout. That way you can have a |
26 |
> "Production" version installed on the same system. Now that I am |
27 |
> testing/debugging, I am comparing catalyst operation and results to |
28 |
> current -9999 code. I am not familiar with using catalyst, so that is |
29 |
> helping me figure out what is wrong. |
30 |
|
31 |
A fair enough. Again, it would be nice if there was a FIXME or |
32 |
something in the commit message so I knew it wasn't destined for |
33 |
master ;). |
34 |
|
35 |
> > 9d752a7 move confdefaults out of main.py |
36 |
> > |
37 |
> > Looks good, except, I'm not sure why you changed from |
38 |
> > `confdefaults.keys()` to `list(confdefaults)` in parse_config() (which |
39 |
> > should probably be living in catalyst.config anyway). |
40 |
> |
41 |
> keeping a separate defaults file can be helpful in importing some info |
42 |
> into different modules while keeping imports to a minimum. Sometimes it |
43 |
> helps prevents circular import problems. At this point I opted for a |
44 |
> separate file, to be determined later if a merge is warranted. |
45 |
|
46 |
ok. |
47 |
|
48 |
> As for list(confdefaults), py3 compatibility. dict.keys() isn't usable |
49 |
> and 2to3 converts it to list(dict)... something about needing to specify |
50 |
> the return type. So is a preemptive change. One less thing to change |
51 |
> later. |
52 |
|
53 |
Really? |
54 |
|
55 |
$ python3.3 -c "a = {1:2, 3:4}; print([x for x in a.keys()])" |
56 |
[1, 3] |
57 |
|
58 |
On the other hand, it might be cleaner to just say: |
59 |
|
60 |
for x in confdefaults: |
61 |
|
62 |
But this should still go into a separate commit. |
63 |
|
64 |
> > c303dae some options cleanup, unifying their use, reducing redundancy. |
65 |
> > |
66 |
> > While I like the general thrust of this, I'd be happier with explicit |
67 |
> > boolean options instead of a set of boolean options. For example: |
68 |
> > |
69 |
> > confdefaults = { |
70 |
> > 'autoresume': False, |
71 |
> > 'ccache': False, |
72 |
> > … |
73 |
> > } |
74 |
> |
75 |
> Yeah, I removed those. They were capitalized versions of the values in |
76 |
> options. So, I optimized them into options becoming a set which |
77 |
> eliminates, the duplication and potential problems by changing the value |
78 |
> of one and not the other. Believe me keeping 2 different lists in sync |
79 |
> can be much more difficult than it seems. It also makes things much |
80 |
> more difficult to debug. (the independent booleans like you suggest can |
81 |
> be considered a list, the other is the options list, set, |
82 |
> string...whatever form it is in) |
83 |
|
84 |
I think I would do something like: |
85 |
|
86 |
import collections as _collections |
87 |
import ConfigParser as _configparser |
88 |
|
89 |
CONFIG = _configparser.ConfigParser(dict_type=_collections.OrderedDict) |
90 |
for setting,value in [ |
91 |
('ccache', str(False))]: |
92 |
CONFIG.set('DEFAULT', setting, value) |
93 |
|
94 |
although OrderedDict doesn't exist in 2.6, where we should probably |
95 |
just fall back to dicts. Use it with: |
96 |
|
97 |
if CONFIG.getboolean('DEFAULT', 'ccache'): |
98 |
… |
99 |
|
100 |
Then there's no duplication to worry about, and you get a |
101 |
configuration object Python developers will be familiar with. |
102 |
|
103 |
> also using member inclusion is faster and prefered compared to other |
104 |
> methods like has_key(). In this case, it is just simpler to use |
105 |
> "options" rather than to individualize them. |
106 |
|
107 |
I don't think option lookup speed will have much impact on catalyst |
108 |
execution speed ;). And with ConfigParser, individual options will |
109 |
require no additional coding. |
110 |
|
111 |
> > Can we use logging instead of print? |
112 |
> |
113 |
> YES!!!!, please :D |
114 |
> |
115 |
> that's been on my wish list too. |
116 |
|
117 |
I can add this if you don't want to. Let me know if you want me to |
118 |
base my patch against `master`, or against something in your branch. |
119 |
|
120 |
> > I think keyword arguments are better, because changes to keywords |
121 |
> > usually occur alongside changes to the argument semantics. A keyword |
122 |
> > mismatch is an obvious fix, while changes due to a semantic shift can |
123 |
> > be more subtle. |
124 |
> |
125 |
> yeah, I was a bit frustrated at that point, debugging code, so chose the |
126 |
> easy way. /me fixes. |
127 |
|
128 |
;). I've certainly been there too. |
129 |
|
130 |
> > 923e8a2 remove trailing slash for consistency in variables and remove |
131 |
> > extra slashes in paths |
132 |
> > |
133 |
> > os.path.join() |
134 |
> |
135 |
> Yes, for sure. But after I debug my current changes. But also most of |
136 |
> those are for the bash side consistency which can not use os.path.join() |
137 |
> and were adding the slashes again at times. |
138 |
> |
139 |
> I am not a bash programmer, so if someone good at the bash stuff wants |
140 |
> to work on those... go for it. I'll try to keep my damage to a minimum. |
141 |
|
142 |
I can look into this, but I think I need a better description of the |
143 |
problem first. Can you give me an example breakage due to using |
144 |
os.path.join? |
145 |
|
146 |
> Also if releng want to recode them in python, that's ok with me ;) |
147 |
|
148 |
I'd be ok with that too, although I don't feel a pressing need for it |
149 |
;). |
150 |
|
151 |
> Thank you for the review, it has been informative. And good to keep me |
152 |
> from any blunders. |
153 |
|
154 |
No problem :D |
155 |
|
156 |
Trevor |
157 |
|
158 |
-- |
159 |
This email may be signed or encrypted with GnuPG (http://www.gnupg.org). |
160 |
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy |