On 22.10.2015 14:24, Tomas Babej wrote: > > > On 10/22/2015 02:15 PM, Petr Spacek wrote: >> On 20.10.2015 17:47, Martin Babinsky wrote: >>> + def check_domainlevel(self, api): >>> + domain_level = dsinstance.get_domain_level(api) >>> + if domain_level > MIN_DOMAIN_LEVEL: >>> + raise RuntimeError( >>> + UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE.format( >>> + command_name=self.command_name, >>> + min_domain_level=MIN_DOMAIN_LEVEL, >>> + curr_domain_level=domain_level) >>> + ) >> >> NACK. >> >> This is very very weird function because it compares two values which are not >> passed as parameters, and also the parameter "api" seems to be unused. >> >> At very least a explanatory doc string is needed, but a new name might be >> even >> better. >> >> Check domain level of what against what? It would be great if function name >> could answer this question. >> > > Also note we have a dedicated exception InvalidDomainLevelError which > should be used in such situations. > > Additionally, I'm not sure if putting this huge blob of text (with > instructions) into the exception is the best way forward, imho we can > either document it somewhere else ('ipa help something?' wiki?) and > reference it here. > > Alternatively, we can just use a logger to log these instructions > instead of passing them in the exception itself.
One more point: + if domain_level > MIN_DOMAIN_LEVEL: + raise RuntimeError( + UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE.format( It is kind of weird that error happens if domain level is greater than some minimal value. Better naming is badly needed. -- Petr^2 Spacek -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code