> usr/src/cmd/installadm/list.py
> ---------------------------------------
>
> 188, 494: Just curious as to why
> print ' '.ljust(width)
> was changed to
> print ' ' * width
> Seems more consistent with other print statements in this function
to leave it
> the "ljust" way.
I changed it because I saw it as the difference between "formatting an
existing string" and "creating a new string".
I'll change it back, if it's important.
>
> 234: strip of the -> strip off the
>
> 365: '-' is length 1, but aliasofwidth is still set to 0 here. Move
363 to below
> the if/else, to 365+?
Adjusted.
>
> 384: Making "No services configured" as an error again would be
consistent with
> other commands, for example ls when it finds no files.
>
This is actually caught much earlier, in do_list, so I'm going to remove
the redundant error checking.
> 518: same comment as for 384.
I disagree. It's possible to chain a list command; e.g. "installadm list
-c -m -p -n <service>"; the lack of clients doesn't indicate an error
with the command - and the SystemExit actually breaks that, so I'll
change this to just return.
>
> 607-614: Idea: since errors will muddy up the IO formatting on the
screen, you
> can maintain a list of errors and dump them to the screen at the end
of the
> "normal" output (but through stderr). This will keep formatting
readable and
> still display errors as they need to be displayed.
Well, these errors are unlikely. And they'll actually get printed out
separate from the printing of the manifests if they do occur, since
we're just gathering information in this function, and only later
dumping out all of that information.
>
> 743: General question: how come some calls to AIService are in
try/except (e.g.
> 560, 343, 827, 974) and others are not (229, 743) ?
Looks like the ones at 229 and 743 deserve some wrappers. I'll add them.
I believe that the current codepaths mean that the unwrapped ones would
never throw a VersionError; but adding the correct except clauses should
future proof them a bit.
>
> 777: Please either add an "else: raise Exception" for the case where
the dbtable
> is not a profile or manifest table (preferred), or else change the
elif on 764
> to an else.
I'm not saying that's not a bad idea, but that's not from ISIM. It's a
programming error if it occurs, and the blanket "except StandardError"
wrapping the whole block makes it the raising of an exception at that
point interesting, but it should be enough to catch programming errors
in the future.
- Keith
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
