On 22.10.2015 14:29, Petr Spacek wrote:
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(
+                    command_name=self.command_name,
+                    min_domain_level=MIN_DOMAIN_LEVEL,
+                    curr_domain_level=domain_level)
+            )

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

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(

It is kind of weird that error happens if domain level is greater than some
minimal value. Better naming is badly needed.

I acked and pushed this patch 2 days ago, and probably my email has been lost forever, so I did bad review, please sent fix as new patch :(

Original patch pushed d81260ef60b64c312e3a164e90ac4faad75c5d82

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to