[Freeipa-devel] [PATCH] 0082/0083 Handle NotFound exception when establishing trust

[Alexander Bokovoy]

On Fri, 12 Oct 2012, Petr Viktorin wrote:
>>>>>Sure. Please share findings of your investigation, then.
>>>>>
>>>>>https://fedorahosted.org/freeipa/ticket/3167
>>>>>
>>>>>Basically, I think you generalized too much. The result is much more
>>>>>complex (=less understandable, maintainable, changeable) than a
>>>>>'\n'.join() when creating the error.
>>>>>
>>>>>>
>>>>>>With the following patch I have this behavior:
>>>>>>>>>from ipalib import errors
>>>>>>>>>raise errors.PublicError(lines=['foo','bar'], format="You've got a
>>>>>>>>>message: %(lines)s")
>>>>>>Traceback (most recent call last):
>>>>>> File "<console>", line 1, in <module>
>>>>>>PublicError: You've got a message: foo
>>>>>>bar
>>>>>>>>>raise errors.OverlapError(names=['foo','bar'])
>>>>>>Traceback (most recent call last):
>>>>>> File "<console>", line 1, in <module>
>>>>>>OverlapError: overlapping arguments and options: ['foo', 'bar']
>>>>>
>>>>>The new patch adds even more complexity. Adding the conversions only
>>>>>to error classes that need them (if any), as opposed to PublicError
>>>>>with an option to turn them off, would be much more straightforward.
>>>>After discussing more with Petr on irc, I came up with the following
>>>>patch: allow `instructions' additional parameter to PublicError() to
>>>>add instructions to an error message.
>>>>
>>>>Trust's error message is converted to show its use.
>>>>
>>>
>>>The rationale:
>>>The need for multi-line error messages was prompted by requests that
>>>IPA should either fix/prevent errors itself, or print out more
>>>detailed instructions for the user.
>>>Automatic joining of lists is problematic, because it repurposes a
>>>standard datatype for the needs of a fairly specific use case.
>>>Converting individual arguments that need it is better than a blanket
>>>approach.
>>>An extra argument for instructions allows us to separate the error
>>>message and instructions in the future, if we need to e.g. add more
>>>complex formatting than the \t.
>>>
>>>
>>>Patch comments:
>>>This needs some tests to verify it's right and prevent regressions.
>>>Please rename the convert_keyword function to e.g. format_instructions.
>>>It's not necessary to add the instructions to self.msg, that one is
>>>not user-friendly.
>>>Please wrap the long line in errors.py.
>>Updated the patch with new test case and fixes. Also split out trust.py
>>changes to a separate patch.
>>
>>Both attached.
>>
>>
>
>This works, however the test regexp doesn't do what it claims 
>(doesn't check if each string is on a separate line).
>Checking the entire message would make the test more straightforward.
>Squash in the attached patch if you agree.
I purposedly went regexp way because of _("Additional instructions"). I
know that our testsuite is not passing when running it localized so it
may be a moot point but avoiding dependency on a localized content in
the test was one specific reason for the approach.

-- 
/ Alexander Bokovoy