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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to