jeanm wrote: > Keith, > > Thanks for the review. Comments are inline. In general, anything that > might effect functionality I'm not going to do now. The risk is too > high and it does pass our standard we have set. Hi Jean,
I agree with your assessment in terms of risk being too high on most points. However, changes such as "== None" to "is None" or "x == False" to "not x" really should be done as part of PEP8 cleaning. The only risk is that of typos, but given the number of instances of the non-PEP8 syntax, I understand the need to defer some/all changes to after the Python2.6 conversion. Some comments below, everything else is ok for now. Thanks, Keith > > Jean > > Keith Mitchell wrote: >> Hi Jean, >> >> As usual, I tend to look 'slightly' beyond what was actually touched >> (though I think I restrained myself almost entirely to PEP8 this >> time). Feel free to tell me certain changes are not in scope and/or >> file bugs for later fixing. >> >> Thanks, >> Keith >> >> General: >> General (non-pep8 note) If everything is going to be a staticmethod >> (as in, for example, DefaultModule and ValidatorModule), this doesn't >> need to be a class. > I talked with Jack about this and here was his response: > "I did this to provide flexibility in the general case, in case for > some reason a default setter or validator needed to save state. > > For DC's case, the methods don't store state. This is why there is no > init method. > > pylint can be appeased by making these methods functions. Instances > of DefaultsModule and ValidationModule can still be created and the > validator and default setter functions can be called the same way they > are now. I wrote a quick-n-dirty program to verify this (below). > > ... And if state is needed in the general case in the future, it is > still accommodated." Understood! Thanks for clarifying. >> >> transfer_defs and dc_defs: defining __all__ would greatly simplify >> the import statements in other modules. >> >> Naming: >> A lot of our functions and modules start with "DC_" (now "dc_"). >> Since they are in the distro_const subdir of osol_install, prepending >> them with dc_ is redundant. Since the names are all getting touched, >> perhaps we could fix this convention? > I'm going to say no to both of these. In the future we should consider > that but it's just to invasive. For __all__, I agree. For name changes, I would consider it more invasive to have to go through 2 sets of name changes here (one now, one later), which is why I brought it up, but I'm ok with deferring as well.
