Hi Jack,
Please see my response below. I removed the items that I have no further
comments on. I also reviewed the updated webrev, and if I still have
comments
about the changes, the line numbers are referring to the updated webrev.
On 03/02/12 23:43, Jack Schwartz wrote:
/usr/src/cmd/text-install/summary.py:
- lines 283-292: Since this is the text installer's summary screen, I
don't
think these lines are lines appropriate? Only lines 279-282 are
appropriate
for text installer summary screen.
OK.
I think the updated webrev is still not quiet right. Line 295 makes the
unnecessary
check to see whether we are in the live_env_boot. The text installer is
only run from
live environments. I do not think there's any need to check.
usr/src/cmd/system-config/summary.py:
- lines 247-250: These few lines are not needed, because if you are
at the sys config tool summary screen, you are not running from
a live env.
Yup, removed. Also removed some extraneous comments.
The updated webrev is still not quiet right. lines 269 and 272 mentions
about
"validation will be attempted on installed systems(s)". If we are running
the sys config tool, we must be running on an installed system, right?
Also, since the sys config tool can only be run from installed systems,
is the message from 267-269 necessary?
usr/src/cmd/system-config/profile/support_info.py:
- Gneral question. Why _is_live_env(), _current_net_exists() and
_write_password_file() functions start with "_" for the name?
They are used only in this file.
OK. Is that a Python convention? I don't remember seeing it in any
other code..
- General comment about this file:
There are 4 classes defined in this file, OCM_Bridge, ASR_Bridge
SupportCreds and SupportInfo. I do not see the need for these
4 separate classes. I belive it's better for their functionalities
to be combined into 1 single class.
From what I can see, SupportCreds and SupportInfo class contains
the same information. The only thing is the SupportInfo class
is a DOC object. Because they contain the same information, you
need to define SupportCreds.save() and SupportCreds.get() functions
to "transfer" the information between them. Also, in the support.py
UI screens, you need to keep track of whether the information
got saved or not. Why do we need the separate SupportCreds class?
I think having the separate classes are very error prone, because
people might forget to "save".
Other modules, such as users.py, take their input into local
variables, call validate() on the local variable values, and then copy
them to the DOC object in on_change_screen(). I assume this was done
for a reason.
I am not quiet sure which example are you trying to follow there. I
looked at users.py.
They are always manipulating DOC objects. The _show() function in
UserScreen() class
either retrieves the user and role value from the DOC. If they don't
exist, they are created
in the DOC. The on_change_screen() function assigns values directly to
the objects in the DOC.
Furthermore, other UI code, such as the other screens in sys-config,
and the code in the text installer, also manipulates values
from the DOC directly.
The SupportCreds class serves the same purpose as the local variables
do in users.py. There are so many fields that I created a
SupportCreds class to store them all. Furthermore, those fields get
manipulated and changed by authentication. Better to commit them to
the DOC together once all of their handling is done. That's why
SupportCreds was created in the first place.
If I can violate that protocol (and please let me know if I can), then
I'm happy to get rid of SupportCreds.
I also looked at other screens in sys config, such as the name services
and timezone screens. All of them uses
values directly from DOC or puts it's values directly to DOC.
The advantage of having all the value in the DOC is that if the program
crashed for some reason. Installers
will dump the content of the whole DOC. So, we can get more information
about state of things if
values are stored.
Furthermore, the OCM_Bridge and ASR_Bridge just provides the
functionality of calling the OCM or ASR command and parse the output.
Those 2 classes doesn't really have any unique attributes.
_has_auth_script or can_authenticate()
Those can easily be bundled into the SupportInfo class.
It needs
all the information it needs to work with from the SupportInfo class.
Therefore, it needs a very long list of arguments and a very long
list of return functions. Why not put all the functionalities in
the SupportInfo class itself, since it is utializing all the data there.
Also, instead of passing back 5 return values, those values
can directly be set on the private variables of the SupportInfo class.
I like the idea of having separate functionality grouped together in a
class, even if the class is not ever instantiated. I also see your
point about long argument lists.
I do not see much value in this case, because having the functionality
in a separate function within
the SupportInfo class is separate enough. The long arguments and long
return value list is very
error prone for future maintenance of the code point of view. Drew was
also telling me that there's
some disadvantage of having separate objects. Unfortunately, I don't
recall the disadvantage at this time.
Most of the long variable list are locals for input fields not in
SupportCreds: proxy and hub fields and encryption key, plus a few
other fields. Again, if I can violate the protocol of manipulating in
local variables, then please let me know. If I can do this, I can
collapse all into a single class.
I am not sure I understand what you mean by "violating the protocol of
manipulating local variables". If you are concerned
about manipulating variables in an DOC object, I don't think there's
anything to worry about. There are a lot of other code
that does that. DOC object is nothing special. It's just an object
that is stored in memory, and it stays around for the whole
duration of the application.
IMO, we just need 1 class for SupportInfo(), and all the manipulations
we need on attributes in that class should just be defined in the
class itself instead of being defined in other classes.
- line 94 and 99-102: if you don't make _has_auth_script a private
variable, you don't even need the "can_authenticate()" function.
I don't see a reason why _has_auth_script need to be a private variable.
It is more object-oriented this way. If you feel strongly I'll change
it though.
I do not see how this is more object oriented. I think much more code is
executed for this simple check with the way things are coded right now.
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
