Hi Clay. On 09/17/09 00:38, Clay Baenziger wrote: > Hi Jack, > Thank you for your catches, refinements and talking things > through. As we discussed over IRC and the phone: > For continuation lines, I had not thought about the difference between > an indentation and continuation line w.r.t. PEP8. I do, however, see a > style which I think is largely accepted (and not based off a fixed > indentation such as we have in C-Style). > Looking at Google's published Python style-guide[1], I see > continuations guided by the above code block. So as you noted in PEP8, > there is three spaces after the first line continuation example, but > that's to align "color" with "width". Further, you can see "emphasis" > is aligned with "color" and under the ValueError "width" aligns inside > the parenthesis next to "I don't think so...". > I think this is the more accepted and proscribed Python line > continuation? While I'm most comfortable with a C-style approach, I'm OK with any approach that does not align a continuation statement with either the line above it or the line below it. Go for it.
I also think we should standardize a style across all of our code, whatever that style is. But that's another discussion for a different time. > Otherwise, I've responded in-line. > Thank you, > Clay > [1]: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Indentation > > > >>>> /usr/src/cmd/installadm/delete-service.py >>>> ----------------------------------------- >>>> >>>> I believe style is modelled after cstyle: 4 spaces for a >>>> continuation line, a >>>> tab (8 spaces) for a new nesting level. DC was done this way, and >>>> a quick >>>> inspection shows pkg was done this way too. Being that python >>>> depends so much >>>> on indentation since it doesn't use {} makes this even more >>>> important. It will >>>> make code more readable. I've made other comments about specific >>>> areas in the >>>> code to point out places where the importance of this shows. >>>> >>>> Having code which is clear and consistent throughout a gate adds to >>>> the quality >>>> of the code. >>> >>> I agree, however, we fly in the face of the Python community which >>> strongly mandates the use of PEP8. Unfortunately, with some of my >>> indentation levels I would be taking 48 of 80 characters following >>> C-Style. I've pinged Ethan and Eric, and plans are afoot to migrate >>> our gate to follow PEP8 space based indentation and 4 space indents. >>> Thank you for catching this and I'm sure there will be more >>> discussion on this. >> > > >>>> 240: () helps make things clearer >>> >>> Remove the wanboot between the parens? >> Never mind. If I run through all the possible cases >> if not True and True >> if not True and False >> if not False and True >> if not False and False >> parentheses don't seem to matter! That is >> if (not A) and B >> if not (A and B) >> yield the same result. >> I'm sure this is an obvious thing to a math wiz, but it is >> fascinating to me... > > Yes, communicative and distributive properties[1] are a bit hard to > grok some days -- I know I still get confused and other algebras like > matrix algebra can really bend the mind. Not sure it's distributive or associative property, because the same isn't true with OR. if (not True) or True -> True if not (True or True) -> False Anyway, I found this to be an interesting diversion... > >>>> 426-427: indentation. >>> >>> I'm not sure what's wrong here, I was following PEP8 which says to >>> use a four space indent regardless. I'm trying to understand how >>> best to proceed with our gate being traditionally a mangled Sun >>> C-Style for non-C code. >> PEP8 (http://python.org/dev/peps/pep-0008/) doesn't say that. >> >> PEP8 says "Use 4 spaces per indentation level" in the "Code lay-out" >> section. To me an indentation level means nesting level, and does not >> include continuation lines. >> >> In the PEP8 document, look at the first "if" in __init__() under >> class Rectangle, and you will see that its continuation lines are not >> indented as much as the next nesting level (raise...). (They use 3 >> spaces for continuations, though I would probably use two.) In the >> paragraph just above that, it says "Make sure to indent the >> continued line appropriately." >> >> Finally, even if the rule would be broken, the document says in the >> section just past the introduction, that one good reason to break a >> particular rule is "when applying the rule would make the code less >> readable..." >> >> Code is less readable when continuation lines and lines of the next >> nesting level are indented the same. Yes, it's my opinion, but I'll >> be you dinner next time I see you that others would agree with me :) > > See the heading discussion >>>> 641: How does service.instance.services get decremented? If this >>>> happens as >>>> processes are terminated, we might need to call waitpid() to wait >>>> for the >>>> os.kill of 629 to take effect before checking >>>> service.instance.services. >>> >>> service.instance.services doesn't change value, it's simply a list >>> of AI services register with SMF -- nothing to do with processes >> OK. I wasn't sure, since 638 says "If we are the last service", >> which to me implies that there were other services which had died or >> been killed, and this is the last one waiting to be terminated. >> Maybe the wording of 638 can be altered? > > Let me try to clarify this OK > >>>> installadm.c >>>> ------------ >>>> >>>> 898-899: The comment doesn't reflect that delete_service >>>> terminates all >>>> processes related to a service, as well as delete the image and files >>>> associated with it. >>> >>> This is intentionally as a comment here is not likely to be updated >>> with code in delete-service.py, I'd rather just point to >>> delete-service.py and let the developer, etc. follow the trail -- >>> nasty but the cost of splitting into multiple pieces until it's all >>> one language. >> Well you do mention -x which is script specific... but OK. >> >> I would reword the comment to say: >> >> Call the SERVICE_DELETE_SCRIPT to delete the service. The script >> will remove the image if the -x arg is passed to it. > > Let me look into this but admittedly you, Evan, Ethan and me all see > this in various ways. OK > >>>> >>>> installadm_common.py >>>> -------------------- >>>> >>>> Not sure how much this file will change when its class is >>>> reimplemented as a >>>> child of dict, but here's a fast review of this file. >>>> >>>> 85: A lot of work gets done in this routine, for it to get called >>>> over and over >>>> again to reprocess the same data / file (I'm thinking __getitem__() >>>> does this.) >>>> Can you check at the beginning whether self.data['lines'], >>>> ...['rawFields'] or >>>> ...['fields'] exists, and just return if things are already set up? >>>> (Another >>>> option: you can add an arg to force a reload, in case you need to >>>> see changes >>>> in the file.) >>> >>> I can check if a file.read is different from the data last read (a >>> partial caching approach) >> If the data gets "changed" when you initialize and when you write(), >> then why not call loadData at those two times only, and just have >> __getitem__() return self.data['fields'][key]) ? Seems much more >> efficient, unless I'm missing something. > > As we talked about there is the threat of having been changed > externally too. However, a check against the hash of the file (as a > string) as last read versus currently is now done (versus whole > reprocessing). Fair enough. If there is an efficient way of checking for external file changes, and running loadData() if a change has been detected, no matter where it came from, then I'm OK with that. Thanks, Jack
