On 11/15/2013 05:03 PM, Tomas Babej wrote: > On 11/07/2013 05:25 PM, Ana Krivokapic wrote: >> Hello, >> >> This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3790. >> >> >> >> _______________________________________________ >> Freeipa-devel mailing list >> [email protected] >> https://www.redhat.com/mailman/listinfo/freeipa-devel > > Looking good.. > > I have two questions: > > > 1.) Nitpick: I'd suggest we rename the save_state(service) and > restore_state(service) to more descriptive > save_service_state/restore_service_state? >
Well, if the argument you are passing to these function does not have a name which suggests it is a service (which it should have anyway), you can do: `save_state(service=x)`. So I don't think `save_service_state(x)` is more readable. > 2.) There are other places in ipa-client-install where we save and restore the > state of the service. Having abstracted that into a function, should we use > this at other places as well? > > I looked at other instances in ipa-client-install where service states are saved and restored. It seems that each of these cases includes some custom logic which does not make it possible to use the two functions I've added. > > -- > Tomas Babej > Associate Software Engeneer | Red Hat | Identity Management > RHCE | Brno Site | IRC: tbabej | freeipa.org -- Regards, Ana Krivokapic Associate Software Engineer FreeIPA team Red Hat Inc.
_______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
