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?
Otherwise, I've responded in-line.
Thank you,
Clay
[1]:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Indentation
On Wed, 16 Sep 2009, Jack Schwartz wrote:
> Hi Clay.
>
> Here's my_reply (your_reply (my_reply (your_review)))) :)
Haha, wow, maybe I see how line comprehension looks now ;)
> I deleted stuff I didn't have further comments on.
>
> On 09/11/09 16:14, Clay Baenziger wrote:
>> Hi Jack,
>> Comments in-line.
>> Thank you,
>> Clay
>>
>> On Tue, 8 Sep 2009, Jack Schwartz wrote:
>>
[snip...]
>>> /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.
> OK, but a rule of thumb which I learned the hard way was that if there are
> too many indentation levels to fit (in 80 spaces using tabs for nesting
> levels) then the code can often be rewritten in a clearer way (by breaking
> large methods into smaller ones, restructuring loops and ifs, and so on).
This is true; I'll look at that as I break things down a bit...
>>
>>> 65, 70: Can instantiate smf.AISCF(FMRI="system/install/server") once and
>>> reuse.
>>
>> Ok, this is done later in the code via service.instance... However, it's a
>> small cost to instantiate upon need. Shall I just do:
>> smfInstance = AISCF(...) and use that for the two calls?
> Sounds good.
>>> 89: I don't understand the comment.
>>
>> Does "return the MACAddress object and options object clarify better?
> The comment currently says:
>
> # return the MAC and options (will except if MAC is not valid)
>
> I don't want to lose info with your change. Maybe you meant:
>
> # return the MAC address and options except if the MAC address is not valid
Yes, good way to put it. Thank you
> Is this accurate?
[snip...]
>>> 203 and other places: Nit: "fn" is quite cryptic for a variable name
>>
>> I'll accept that if you can come up with a good variable name for filename
>> which is reasonably short?
> How about "filename"? It has as many letters as "function" does...
Yes, I think I'm just being overly-zealous about avoiding line wraps. I'll
use filename.
>>> 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.
I tend to find extraneous parens to confuse as to if the logic was intended
to be something else at one point too though.
[1]:
http://www.wtamu.edu/academic/anns/mps/math/mathlab/beg_algebra/beg_alg_tut8_property.htm
[snip...]
>>> 340-344 Please explain the logic here.
>>
>> This iterates down the directory structure adding any directory which
>> doesn't contain something not being removed to the list of objects to
>> remove. (E.g. if we start with dir =
>> /var/ai/image-server/var/ai/clayb_ai_sparc, then we see if
>> /var/ai/image-server/var/ai/ has anything other than clayb_ai_sparc in it,
>> and if /var/ai/image-service/var/ has anything other than ai, etc. The
>> /var/ai/image-service directory will have the various image-server files in
>> it and will break the while loop condition.
> OK. Thanks for the explanation. Not sure if the directory will be as empty
> as expected, though. I think what you say above is more accurate: that it
> doesn't contain something not being removed. Won't the files to be removed
> from it only have been appended to the "files" list but not yet deleted? If
> not, then the len(listdir()) check won't be accurate, right?
As per our phone conversation I will clarify the comment that we are
traversing down from a file being deleted. Further, as per our
conversation and talking with Jean I will see if a check for /var/ai is
appropriate here.
>> paths=[os.readlink(l) for l in links]
>> # ensure microRootGrub is not shared
>> if (paths.count(microRootGrub) == 1):
> Just checking: you only want to check links when checking that microRootGrub
> isn't shared, right?
Yes, this is only checking sym-links as hard-links won't go away and
another file can't have the same name.
>>> 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
[snip...]
>>> 474: I believe calling /tftpboot/rm.<service-name> will remove some files
>>> but
>>> not those which get appended to filesToRemove list (which get removed
>>> later).
>>> Is this correct? Whatever the relationship between the filesToRemove list
>>> and
>>> the files in /tftpboot/rm.<service-name>, please add a comment explaining
>>> it.
>>
>> So, the files which rm.<service name> should remove are talked about in the
>> lines 487-496. How should I better draw attention to that?
> Put a colon at the end of 486. This should make things clearer, so that even
> a person who's tired (as I probably was that day) can understand the comment
> better.
Ah cool, I see I'll tie 486/487 together better.
>>> 478, 482: Can optimize with a single string for "rm." +
>>> service.serviceName
>>
>> What's being optimized? I don't understand.
> rm_script = os.path.join(baseDir, "rm." + service.serviceName)
> os.chmod(rm_script, stat.xxx | stat.yyy)
> com.run_cmd({"cmd" : [rm_script]})
Ok
>>
>>> 554: apply() has ben deprecated since 2.3.
>>
>> Yes, I wasn't sure I could do object(service) but I believe that should be
>> okay.
> Docs say it should be equivalent.
Cool :)
[snip...]
>>
>>> 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
>>> 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.
>>>
>>> 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).
>>
>>
>>
>>
>>> 102-103: Can be optimized: the same line gets split() twice.
>>
>> Optimized at the potential cost of memory and garbage collection. Let me
>> weigh the choice or see if I can understand the Python internals a bit
>> better.
> Hmmm... OK. This is probably splitting hairs, so don't worry about it if too
> much trouble.
The D-Trace toolkit isn't happy on my laptop right now, however, it has
some D-Trace scripts pre-written to investigate Python which might answer
this. Regardless, its gone in making the file classes more generic as
Ethan's recently asked.
