Tomas,
Thanks for fixing this. A few comments ...
61 - Did you perhaps mean descendants here, not ancestors?
87, 300, 425, 502 - nit - Since these classes don't actually do the
printing, could they be renamed to:
CriteriaPrintObject
ManifestPrintObject
ProfilePrintObject
ServicePrintObject
91 - Here, and may other places throughout, you have a typo "formated"
-> "formatted"
116 - Why is this argument called 'cjust', and described as the
justification at 120? It's looks like it is the size of this object's
width. Could it be named 'cwidth' instead? This same comment goes for
all instances of mjust, pjust, just, and cjust throughout the remainder
of the code.
116,128,139 - just a nit here but the fact that the argument itself is
already aptly named 'indent' seems descriptive enough to signify what is
happening in the function where it is used, so could these function
names be shortened and the 'indent' portion removed? Also, because
these methods don't really print anything, could they be renamed, perhaps:
get_header()
get_underline()
get_lines() ??
282-297 - Couldn't you just call AIdb.getTableCriteria() with
humanOutput=True at line 282 to prevent having to call it again at 292?
302 - nits -"manifest" -> "a manifest" ; "contains also list" -> "also
contains a list"
427 - nits - "profile" -> "a profile" ; "contains also criteria" ->
"also contains criteria"
502 - Should the ServicePrint object be a subclass of PrintObject? It
seems like it has at least two methods that are already defined
identically for PrintObject.
631,771 - This function doesn't print anything so this should be
"Prints" -> "Returns"
631,771 - typo - lengt -> length
889 - Is it possible to make this class support both -- printing
manifests for all services, or for just one? To me, it seems awkward
for the code at 1487,1491 to use two different classes to print one case
or the other. Why can't this class just take an optional service_name
argument for cases where only one service needs to be printed?
thanks,
-ethan
On 01/20/12 04:10, Tomas Dzik wrote:
Hi all,
I would like to kindly ask whether someone has some spare time to
review my changes.
I know that these changes are quite substantial.
Best regards,
Tomas D.
Dne 30.12.11 15:46, Tomas Dzik napsal(a):
Hi all,
I would like to ask you for a code review for:
7120099 - "installadm list -p" should show criteria
This fix contains also changes to the way how "installadm list -m"
works.
Webrev:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7120099/
Testing:
1) Sources are pep8 clean
2) Test suite in the gate pass
3) I created several services with different manifests (Default, Active,
Inactive) with several sets of criteria and verified that these are
printed as before
4) I also created profiles with different criteria and verified how they
are printed.
5) I tried to change the version of installed service and verified that
code correctly handles this exception.
I changed a little bit format how ignored criteria are printed (as
suggested in one of the previous code reviews).
Instead:
# installadm list -m -n default-i386
Manifest Status Criteria
-------- ------ --------
orig_default Default (Ignored: ipv4 = 192.168.1.2)
(Ignored: mem = 2048 MB - unbounded)
I now print:
# installadm list -m -n default-i386
Manifest Status Criteria
-------- ------ --------
orig_default Default (Ignored:
ipv4 = 192.168.1.2
mem = 2048 MB - unbounded
)
Best regards,
Tomas D.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
